On Fri, Nov 29, 2024 at 02:47:59PM +0100, David Hildenbrand wrote:
> On 29.11.24 14:38, Lorenzo Stoakes wrote:
> > On Fri, Nov 29, 2024 at 02:24:24PM +0100, David Hildenbrand wrote:
> > > On 29.11.24 14:19, Lorenzo Stoakes wrote:
> > > > On Fri, Nov 29, 2024 at 02:12:23PM +0100, David Hildenbrand wrote:
> > > > > On 29.11.24 14:02, Lorenzo Stoakes wrote:
> > > > > > On Fri, Nov 29, 2024 at 01:59:01PM +0100, David Hildenbrand wrote:
> > > > > > > On 29.11.24 13:55, Lorenzo Stoakes wrote:
> > > > > > > > On Fri, Nov 29, 2024 at 01:45:42PM +0100, David Hildenbrand wrote:
> > > > > > > > > On 29.11.24 13:26, Peter Zijlstra wrote:
> > > > > > > > > > On Fri, Nov 29, 2024 at 01:12:57PM +0100, David Hildenbrand wrote:
> > > > > > > > > >
> > > > > > > > > > > Well, I think we simply will want vm_insert_pages_prot() that stops treating
> > > > > > > > > > > these things like folios :) . *likely* we'd want a distinct memdesc/type.
> > > > > > > > > > >
> > > > > > > > > > > We could start that work right now by making some user (iouring,
> > > > > > > > > > > ring_buffer) set a new page->_type, and checking that in
> > > > > > > > > > > vm_insert_pages_prot() + vm_normal_page(). If set, don't touch the refcount
> > > > > > > > > > > and the mapcount.
> > > > > > > > > > >
> > > > > > > > > > > Because then, we can just make all the relevant drivers set the type, refuse
> > > > > > > > > > > in vm_insert_pages_prot() anything that doesn't have the type set, and
> > > > > > > > > > > refuse in vm_normal_page() any pages with this memdesc.
> > > > > > > > > > >
> > > > > > > > > > > Maybe we'd have to teach CoW to copy from such pages, maybe not. GUP of
> > > > > > > > > > > these things will stop working, I hope that is not a problem.
> > > > > > > > > >
> > > > > > > > > > Well... perf-tool likes to call write() upon these pages in order to
> > > > > > > > > > write out the data from the mmap() into a file.
> > > > > > > >
> > > > > > > > I'm confused about what you mean, write() using the fd should work fine, how
> > > > > > > > would they interact with the mmap? I mean be making a silly mistake here
> > > > > > >
> > > > > > > write() to file from the mmap()'ed address range to *some* file.
> > > > > > >
> > > > > >
> > > > > > Yeah sorry my brain melted down briefly, for some reason was thinking of read()
> > > > > > writing into the buffer...
> > > > > >
> > > > > > > This will GUP the pages you inserted.
> > > > > > >
> > > > > > > GUP does not work on PFNMAP.
> > > > > >
> > > > > > Well it _does_ if struct page **pages is set to NULL :)
> > > > >
> > > > > Hm? :)
> > > > >
> > > > > check_vma_flags() unconditionally refuses VM_PFNMAP.
> > > >
> > > > Ha, funny with my name all over git blame there... ok yup missed this, the
> > > > vm_normal_page() == NULL stuff must but for mixed map (and those other weird
> > > > cases I think you can get0...
> > > >
> > > > Well good. Where is write() invoking GUP? I'm kind of surprised it's not just
> > > > using uaccess?
> > > >
> > > > One thing to note is I did run all the perf tests with no issues whatsoever. You
> > > > would _think_ this would have come up...
> > > >
> > > > I'm editing some test code to explicitly write() from the buffer anyway to see.
> >
> > I just tested it and write() works fine, it uses uaccess afaict as part of the
> > lib/iov_iter.c code:
> >
> > generic_perform_write()
> > -> copy_folio_from_iter_atomic()
> > -> copy_page_from_iter_atomic()
> > -> __copy_from_iter()
> > -> copy_from_user_iter()
> > -> raw_copy_from_user()
> > -> copy_user_generic()
> > -> [uaccess asm]
> >
>
> Ah yes. O_DIRECT is the problematic bit I suspect, which will use GUP.
>
> Ptrace access and friends should also no longer work on these pages, likely
> that's tolerable.
Yeah Peter can interject if not, but I'd be _very_ surprised if anybody expects
to be able to ptrace perf counter mappings in another process (it'd be kind of
totally insane to do that anyway since it's a ring buffer that needs special
handing anyway).
>
> > > >
> > > > If we can't do pfnmap, and we definitely can't do mixedmap (because it's
> > > > basically entirely equivalent in every way to just faulting in the pages as
> > > > before and requires the same hacks) then I will have to go back to the drawing
> > > > board or somehow change the faulting code.
> > > >
> > > > This really sucks.
> > > >
> > > > I'm not quite sure I even understand why we don't allow GUP used _just for
> > > > pinning_ on VM_PFNMAP when it is -in effect- already pinned on assumption
> > > > whatever mapped it will maintain the lifetime.
> > > >
> > > > What a mess...
> > >
> > > Because VM_PFNMAP is dangerous as hell. To get a feeling for that (and also why I
> > > raised my refcounting comment earlier) just read recent:
> > >
> > > commit 79a61cc3fc0466ad2b7b89618a6157785f0293b3
> > > Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > Date: Wed Sep 11 17:11:23 2024 -0700
> > >
> > > mm: avoid leaving partial pfn mappings around in error case
> > > As Jann points out, PFN mappings are special, because unlike normal
> > > memory mappings, there is no lifetime information associated with the
> > > mapping - it is just a raw mapping of PFNs with no reference counting of
> > > a 'struct page'.
> > >
> >
> > I'm _very_ aware of this, having worked extensively on things around this kind
> > of issue recently (was a little bit involved with the above one too :), and
> > explicitly zap on error in this patch to ensure we leave no broken code paths.
> >
> > I agree it's horrible, but we need to have a way of mapping this memory without
> > having to 'trick' the faulting mechanism to behave correctly.
>
> What's completely "surprising" to me is, if there is no page_mkwrite, but
> the VMA is writable, then just map the PTE writable ...
I've had a number of surprises on this journey :)
To make sure I understand what you mean do you mean the whole:
do_wp_page()
-> wp_page_shared()
-> if (page_mkwrite) { ... } else { wp_page_reuse(); }
-> wp_page_reuse()
-> maybe_mkwrite() [hey VM_WRITE is set, so yes make writable!]
Path?
Yes this surprised me too... so you can't just say 'hey map this read-only' and
get what you want, because the kernel thinks it's somehow spuriously read-only
mapped and just needs correction (probably there are situations where this is
necessary).
It has to be explicitly CoW in VMA flags or implement page_mkwrite() (or if
!vm_normal_page() then pfn_mkwrite().
You aren't saved with !vm_normal_page() mappings either without a pfn_mkwrite()
because:
do_wp_page()
-> wp_pfn_shared()
-> if (pfn_mkwrite) { ... } else { wp_page_reuse() }
etc.
So it's the only game in town...
>
> >
> > At least in perf's case, we're safe, because we ref count in open/close of VMA's.
> >
> > This is a special case due to the R/W, R/O thing.
> >
> > In reference to that - you said in another email about mapping one part as a
> > separate R/W VMA and another as a separate R/O VMA, problem is all of the perf
> > code is set up with its own reference counting mechanism and it's not allowed to
> > split/merge etc., so we'd have to totally rework all of that to make that work
> > and correctly refcount things.
> >
> > It'd be a big task. I don't think that's a reasonable thing to put effort into
> > at this time...
> >
> > Also who knows if there's somebody, somewhere who _relies_ somehow on this being
> > a single mapping...
>
> The main issue here really is that we allow R/O pages in VM_WRITE VMAs, and
> want to make write faults fail :(
>
> What an absolute mess, yeah, without some more core changes
> vm_insert_pages() cannot be used, unfortunately.
Yes indeed, sadly. Maybe an idea for a series, as memdesc's are at the very
least a long way away.
>
> --
> Cheers,
>
> David / dhildenb
>
So overall - given all the above and the fact that the alternatives are _even
worse_ (having to spuriously folio lock if that'll even work, totally
unnecessary and semantically incorrect folio-fication or a complete rework of
mapping) - while you might be sicked by this use of the VM_PFNMAP - are you ok
with the patch, all things considered? :)
