Message-ID: <f60722d4-1474-4876-9291-5450c7192bd3@lucifer.local>
Date:   Fri, 28 Apr 2023 15:55:58 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jason Gunthorpe <jgg@...pe.ca>, Jens Axboe <axboe@...nel.dk>,
        Matthew Wilcox <willy@...radead.org>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Leon Romanovsky <leon@...nel.org>,
        Christian Benvenuti <benve@...co.com>,
        Nelson Escobar <neescoba@...co.com>,
        Bernard Metzler <bmt@...ich.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Bjorn Topel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Christian Brauner <brauner@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        Oleg Nesterov <oleg@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        John Hubbard <jhubbard@...dia.com>, Jan Kara <jack@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Pavel Begunkov <asml.silence@...il.com>,
        Mika Penttila <mpenttil@...hat.com>,
        David Howells <dhowells@...hat.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings
 by default

On Fri, Apr 28, 2023 at 04:20:46PM +0200, David Hildenbrand wrote:
> Sorry for jumping in late, I'm on vacation :)
>
> On 28.04.23 01:42, Lorenzo Stoakes wrote:
> > Writing to file-backed mappings which require folio dirty tracking using
> > GUP is a fundamentally broken operation, as kernel write access to GUP
> > mappings do not adhere to the semantics expected by a file system.
> >
> > A GUP caller uses the direct mapping to access the folio, which does not
> > cause write notify to trigger, nor does it enforce that the caller marks
> > the folio dirty.
>
> How should we enforce it? It would be a BUG in the GUP user.

There was discussion about holding locks when passing back pages but it's a
sticky question no doubt, but a bit out of scope here. This is more
documenting that this is a thing.

>
> >
> > The problem arises when, after an initial write to the folio, writeback
> > results in the folio being cleaned and then the caller, via the GUP
> > interface, writes to the folio again.
> >
> > As a result of the use of this secondary, direct, mapping to the folio no
> > write notify will occur, and if the caller does mark the folio dirty, this
> > will be done so unexpectedly.
>
> Right, in mprotect() code we only allow upgrading write permissions in this
> case if the pte is dirty, so we always go via the pagefault path.
>

In my ideal world we'd somehow do remote accesses via a kthread_use_mm()
style approach and page fault in every time. That's again a bit out of
scope though...

> >
> > For example, consider the following scenario:-
> >
> > 1. A folio is written to via GUP which write-faults the memory, notifying
> >     the file system and dirtying the folio.
> > 2. Later, writeback is triggered, resulting in the folio being cleaned and
> >     the PTE being marked read-only.
>
>
> How would that be triggered? Would that writeback triggered by e.g., fsync
> that Jan tried to tackle recently?

Yes or just perioditic writeback threads. The folio is dirty and needs
writeback, so at some point that'll happen. Obviously fsync/msync could
cause it too (I may be missing something here :)

>
>
> > 3. The GUP caller writes to the folio, as it is mapped read/write via the
> >     direct mapping.
> > 4. The GUP caller, now done with the page, unpins it and sets it dirty
> >     (though it does not have to).
> >
> > This results in both data being written to a folio without writenotify, and
> > the folio being dirtied unexpectedly (if the caller decides to do so).
> >
> > This issue was first reported by Jan Kara [1] in 2018, where the problem
> > resulted in file system crashes.
> >
> > This is only relevant when the mappings are file-backed and the underlying
> > file system requires folio dirty tracking. File systems which do not, such
> > as shmem or hugetlb, are not at risk and therefore can be written to
> > without issue.
> >
> > Unfortunately this limitation of GUP has been present for some time and
> > requires future rework of the GUP API in order to provide correct write
> > access to such mappings.
> >
> > However, for the time being we introduce this check to prevent the most
> > egregious case of this occurring, use of the FOLL_LONGTERM pin.
> >
> > These mappings are considerably more likely to be written to after
> > folios are cleaned and thus simply must not be permitted to do so.
> >
> > As part of this change we separate out vma_needs_dirty_tracking() as a
> > helper function to determine this which is distinct from
> > vma_wants_writenotify() which is specific to determining which PTE flags to
> > set.
> >
> > [1]:https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz/
> >
>
>
> This change has the potential to break existing setups. Simple example:
> libvirt domains configured for file-backed VM memory that also has a vfio
> device configured. It can easily be configured by users (evolving VM
> configuration, copy-paste etc.). And it works from a VM perspective, because
> the guest memory is essentially stale once the VM is shutdown and the pages
> were unpinned. At least we're not concerned about stale data on disk.
>
> With your changes, such VMs would no longer start, breaking existing user
> setups with a kernel update.

Which vfio vm_ops are we talking about? vfio_pci_mmap_ops for example
doesn't specify page_mkwrite or pfn_mkwrite. Unless you mean some arbitrary
file system in the guest?

I may well be missing context on this so forgive me if I'm being a little
dumb here, but it'd be good to get a specific example.

>
> I don't really see a lot of reasons to perform this change now. It's been
> known to be problematic for a long time. People are working on a fix (I see
> Jan is already CCed, CCing Dave and Christop). FOLL_LONGTERM check is only
> handling some of the problematic cases, so it's not even a complete blocker.

I am not a huge fan of the commentary along the lines of 'I don't really
see a lot of reasons to make this change' when people have gone to lengths
to posit reasons as to why this change is being made, by all means disagree
but it's more helpful if it's of the form 'you say we should do this for
reasons X, Y, Z I disagree for reasons a, b, c' (which you've done
elsewhere).

Also those people working on a longer term fix are supporting this change
;) I think to think that I myself may also contribute to this fix longer
term, perhaps.

>
> I know, Jason und John will disagree, but I don't think we want to be very
> careful with changing the default.
>
> Sure, we could warn, or convert individual users using a flag (io_uring).
> But maybe we should invest more energy on a fix?

This is proactively blocking a cleanup (eliminating vmas) that I believe
will be useful in moving things forward. I am not against an opt-in option
(I have been responding to community feedback in adapting my approach),
which is the way I implemented it all the way back then :)

But given we know this is both entirely broken and a potential security
issue, and FOLL_LONGTERM is about as egregious as you can get (user
explicitly saying they'll hold write access indefinitely) I feel it is an
important improvement and makes clear that this is not an acceptable usage.

I see Jason has said more on this also :)

>
>
>
>
> > Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> > Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
> > ---
> >   include/linux/mm.h |  1 +
> >   mm/gup.c           | 41 ++++++++++++++++++++++++++++++++++++++++-
> >   mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
> >   3 files changed, 68 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 37554b08bb28..f7da02fc89c6 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2433,6 +2433,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
> >   #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
> >   					    MM_CP_UFFD_WP_RESOLVE)
> >
> > +bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
> >   int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
> >   static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
> >   {
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 1f72a717232b..d36a5db9feb1 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -959,16 +959,51 @@ static int faultin_page(struct vm_area_struct *vma,
> >   	return 0;
> >   }
> >
> > +/*
> > + * Writing to file-backed mappings which require folio dirty tracking using GUP
> > + * is a fundamentally broken operation, as kernel write access to GUP mappings
> > + * do not adhere to the semantics expected by a file system.
> > + *
> > + * Consider the following scenario:-
> > + *
> > + * 1. A folio is written to via GUP which write-faults the memory, notifying
> > + *    the file system and dirtying the folio.
> > + * 2. Later, writeback is triggered, resulting in the folio being cleaned and
> > + *    the PTE being marked read-only.
> > + * 3. The GUP caller writes to the folio, as it is mapped read/write via the
> > + *    direct mapping.
> > + * 4. The GUP caller, now done with the page, unpins it and sets it dirty
> > + *    (though it does not have to).
> > + *
> > + * This results in both data being written to a folio without writenotify, and
> > + * the folio being dirtied unexpectedly (if the caller decides to do so).
> > + */
> > +static bool writeable_file_mapping_allowed(struct vm_area_struct *vma,
> > +					   unsigned long gup_flags)
> > +{
> > +	/* If we aren't pinning then no problematic write can occur. */
> > +	if (!(gup_flags & (FOLL_GET | FOLL_PIN)))
> > +		return true;
>
> FOLL_LONGTERM only applies to FOLL_PIN. This check can be dropped.

I understand that of course (well maybe not of course, but I mean I do, I
have oodles of diagrams referencing this int he book :) This is intended to
document the fact that the check isn't relevant if we don't pin at all,
e.g. reading this you see:-

- (implicit) if not writing or anon we're good
- if not pin we're good
- ok we are only currently checking one especially egregious case
- finally, perform the dirty tracking check.

So this is intentional.

>
> > +
> > +	/* We limit this check to the most egregious case - a long term pin. */
> > +	if (!(gup_flags & FOLL_LONGTERM))
> > +		return true;
> > +
> > +	/* If the VMA requires dirty tracking then GUP will be problematic. */
> > +	return vma_needs_dirty_tracking(vma);
> > +}
> > +
> >   static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
> >   {
> >   	vm_flags_t vm_flags = vma->vm_flags;
> >   	int write = (gup_flags & FOLL_WRITE);
> >   	int foreign = (gup_flags & FOLL_REMOTE);
> > +	bool vma_anon = vma_is_anonymous(vma);
> >
> >   	if (vm_flags & (VM_IO | VM_PFNMAP))
> >   		return -EFAULT;
> >
> > -	if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma))
> > +	if ((gup_flags & FOLL_ANON) && !vma_anon)
> >   		return -EFAULT;
> >
> >   	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
> > @@ -978,6 +1013,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
> >   		return -EFAULT;
> >
> >   	if (write) {
> > +		if (!vma_anon &&
> > +		    !writeable_file_mapping_allowed(vma, gup_flags))
> > +			return -EFAULT;
> > +
> >   		if (!(vm_flags & VM_WRITE)) {
> >   			if (!(gup_flags & FOLL_FORCE))
> >   				return -EFAULT;
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 536bbb8fa0ae..7b6344d1832a 100644
> > --- a/mm/mmap.c
>
>
> I'm probably missing something, why don't we have to handle GUP-fast (having
> said that, it's hard to handle ;) )? The sequence you describe above should
> apply to GUP-fast as well, no?
>
> 1) Pin writable mapped page using GUP-fast
> 2) Trigger writeback
> 3) Write to page via pin
> 4) Unpin and set dirty

You're right, and this is an excellent point. I worry about other GUP use
cases too, but we're a bit out of luck there because we don't get to check
the VMA _at all_ (which opens yet another Pandora's box about how safe it
is to do unlocked pinning :)

But again, this comes down to the fact we're trying to make things
_incrementally__ better rather than throwing our hands up and saying one
day my ship will come in...

>
>
> --
> Thanks,
>
> David / dhildenb
>
>

Powered by blists - more mailing lists