by John Hubbard

[permalink] [raw]

Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default

On 4/27/23 15:31, Lorenzo Stoakes wrote:
...
>> bool file_backed = !vma_is_anonymous(vma);
>>
>> would lead to a slightly better reading experience below.
>
> Well you see, I'm not so sure about that, because vma_is_anonymous() checks
> vm_ops == NULL not vm_file == NULL which can be the case for a special
> mapping like VDSO that is not in fact file-backed :) the horror, the
> horror.
>

Yes, you are right. It looks like vma_anon is a better name here,
after all.

...
>> ...and now we call it again. I think once should be enough, though.
>
> Right, this was intentional (I think I mentioned it in the revision
> notes?), because there is a conundrum here - the invocation from
> vma_wants_writenotify() needs to check this _first_ before performing the
> _other_ checks in vma_needs_dirty_tracking(), but external calls need all
> the checks. It'd be ugly to pass a boolean to see if we should check this
> or not, and it's hardly an egregious duplication for the _computer_
> (something likely in a cache line != NULL) which aids readability and
> reduces duplication for the _reader_ of the code for a path that is
> inherently slow (likely going to fault in pages etc.)
>
> I think it'd be confusing to have yet another split into
> vma_can_track_dirty() or whatever because then suddenly for the check to be
> meaningful you have to _always_ check 2 things.
>
> Other options like passing an output parameter or returning something other
> than boolean are equally distasteful.

Agreed. (And yes, I overlooked that discussion in the version notes.)

>
>>
>> Also, with the exception of that double call to
>> vm_ops_needs_writenotify(), these changes to mmap.c are code cleanup
>> that has the same behavior as before. As such, it's better to separate
>> them out from this patch whose goal is very much to change behavior.
>
> It's not really cleanup, it's separating out some of the logic explicitly
> to be used in this new context, without which the separation would not be
> useful, so I feel it's a bit over the top to turn a small single patch into
> two simply to avoid this.
>

Sure, OK.

>>
>
> Thanks for the review, I will respin with the suggestions (other than ones
> I don't quite agree with as explained above) and a clearer description in
> line with Mika's suggestions.
>
> Hopefully we can move closer to this actually getting some
> reviewed/acked-by tags soon :)

thanks,
--
John Hubbard
NVIDIA

2023-04-27 23:14:19

by Lorenzo Stoakes

[permalink] [raw]

Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default

On Wed, Apr 26, 2023 at 05:52:09PM -0700, John Hubbard wrote:
> On 4/25/23 16:15, Lorenzo Stoakes wrote:
> > GUP does not correctly implement write-notify semantics, nor does it
> > guarantee that the underlying pages are correctly dirtied, which could lead
> > to a kernel oops or data corruption when writing to file-backed mappings.
> >
> > 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.
> >
> > In the meantime, we add a check for the most broken GUP case -
> > FOLL_LONGTERM - which really under no circumstances can safely access
> > dirty-tracked file mappings.
> >
>
> Hi Lorenzo,
>
> As I mentioned in a sub-thread [1], it would be a nice touch to include
> your more detailed write-up, and a link to Jan Kara's original report,
> here.

Ack will do.

>
>
> > 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.
>
> This, I think should go in a separate cleanup patch, because it is
> (nearly) the same behavior. More notes below on this.
>
> More notes below:
>
> >
> > Suggested-by: Jason Gunthorpe <[email protected]>
> > Signed-off-by: Lorenzo Stoakes <[email protected]>
> > ---
> > v4:
> > - Split out vma_needs_dirty_tracking() from vma_wants_writenotify() to reduce
> > duplication and update to use this in the GUP check. Note that both separately
> > check vm_ops_needs_writenotify() as the latter needs to test this before the
> > vm_pgprot_modify() test, resulting in vma_wants_writenotify() checking this
> > twice, however it is such a small check this should not be egregious.
> >
> > v3:
> > - Rebased on latest mm-unstable as of 24th April 2023.
> > - Explicitly check whether file system requires folio dirtying. Note that
> > vma_wants_writenotify() could not be used directly as it is very much focused
> > on determining if the PTE r/w should be set (e.g. assuming private mapping
> > does not require it as already set, soft dirty considerations).
> > - Tested code against shmem and hugetlb mappings - confirmed that these are not
> > disallowed by the check.
> > - Eliminate FOLL_ALLOW_BROKEN_FILE_MAPPING flag and instead perform check only
> > for FOLL_LONGTERM pins.
> > - As a result, limit check to internal GUP code.
> > https://lore.kernel.org/all/23c19e27ef0745f6d3125976e047ee0da62569d4.1682406295.git.lstoakes@gmail.com/
> >
> > v2:
> > - Add accidentally excluded ptrace_access_vm() use of
> > FOLL_ALLOW_BROKEN_FILE_MAPPING.
> > - Tweak commit message.
> > https://lore.kernel.org/all/c8ee7e02d3d4f50bb3e40855c53bda39eec85b7d.1682321768.git.lstoakes@gmail.com/
> >
> > v1:
> > https://lore.kernel.org/all/f86dc089b460c80805e321747b0898fd1efe93d7.1682168199.git.lstoakes@gmail.com/
> >
> > include/linux/mm.h | 1 +
> > mm/gup.c | 26 +++++++++++++++++++++++++-
> > mm/mmap.c | 37 ++++++++++++++++++++++++++++---------
> > 3 files changed, 54 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..53652453037c 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -959,16 +959,37 @@ 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
> > + * may not adhere to the semantics expected by a file system.
> > + */
> > +static inline bool can_write_file_mapping(struct vm_area_struct *vma,
> > + unsigned long gup_flags)
>
> Perhaps name this:
> writeable_file_mapping_allowed()
>
> ? "can" is more about "is this possible", whereas the goal here is
> to express, "should this be allowed".

Yes I struggled a bit with how best to name this, I think your suggestion
is better

>
> Also a silly tiny nit: let's omit the "inline" keyword, down here in this
> .c file, and let the compiler work that out instead. "static" should suffice.

Ack, I saw some inconsistency in gup.c about this so wondered which way to
go, obviously it's not really all that useful here (compiler will inline if
it wants)

>
> > +{
> > + /* If we aren't pinning then no problematic write can occur. */
> > + if (!(gup_flags & (FOLL_GET | FOLL_PIN)))
> > + return true;
> > +
> > + /* 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);
>
> This name:
>
> bool file_backed = !vma_is_anonymous(vma);
>
> would lead to a slightly better reading experience below.

Well you see, I'm not so sure about that, because vma_is_anonymous() checks
vm_ops == NULL not vm_file == NULL which can be the case for a special
mapping like VDSO that is not in fact file-backed :) the horror, the
horror.

>
> Sorry for the small naming and documentation comments here,
> it's just what I do. :)
>
>
> > 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 +999,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
> > return -EFAULT;
> > if (write) {
> > + if (!vma_anon && !can_write_file_mapping(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..aac638dd22cf 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1475,6 +1475,32 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
> > }
> > #endif /* __ARCH_WANT_SYS_OLD_MMAP */
> > +/* Do VMA operations imply write notify is required? */
> > +static inline bool vm_ops_needs_writenotify(
>
> This "inline" should also be omitted, imho.
>

Same comment as before as to why I did that, but ack

> > + const struct vm_operations_struct *vm_ops)
> > +{
> > + return vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite);
> > +}
> > +
> > +/*
> > + * Does this VMA require the underlying folios to have their dirty state
> > + * tracked?
> > + */
> > +bool vma_needs_dirty_tracking(struct vm_area_struct *vma)
> > +{
> > + /* Does the filesystem need to be notified? */
> > + if (vm_ops_needs_writenotify(vma->vm_ops))
> > + return true;
> > +
> > + /* Specialty mapping? */
> > + if (vma->vm_flags & VM_PFNMAP)
> > + return false;
> > +
> > + /* Can the mapping track the dirty pages? */
> > + return vma->vm_file && vma->vm_file->f_mapping &&
> > + mapping_can_writeback(vma->vm_file->f_mapping);
> > +}
> > +
> > /*
> > * Some shared mappings will want the pages marked read-only
> > * to track write events. If so, we'll downgrade vm_page_prot
> > @@ -1484,14 +1510,13 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
> > int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)
> > {
> > vm_flags_t vm_flags = vma->vm_flags;
> > - const struct vm_operations_struct *vm_ops = vma->vm_ops;
> > /* If it was private or non-writable, the write bit is already clear */
> > if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED)))
> > return 0;
> > /* The backer wishes to know when pages are first written to? */
> > - if (vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite))
> > + if (vm_ops_needs_writenotify(vma->vm_ops))
>
> OK, so here we are calling vm_ops_needs_writenotify(), that's the
> first call. And then...
>
> > return 1;
> > /* The open routine did something to the protections that pgprot_modify
> > @@ -1511,13 +1536,7 @@ int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot)
> > if (userfaultfd_wp(vma))
> > return 1;
> > - /* Specialty mapping? */
> > - if (vm_flags & VM_PFNMAP)
> > - return 0;
> > -
> > - /* Can the mapping track the dirty pages? */
> > - return vma->vm_file && vma->vm_file->f_mapping &&
> > - mapping_can_writeback(vma->vm_file->f_mapping);
> > + return vma_needs_dirty_tracking(vma);
>
> ...and now we call it again. I think once should be enough, though.

Right, this was intentional (I think I mentioned it in the revision
notes?), because there is a conundrum here - the invocation from
vma_wants_writenotify() needs to check this _first_ before performing the
_other_ checks in vma_needs_dirty_tracking(), but external calls need all
the checks. It'd be ugly to pass a boolean to see if we should check this
or not, and it's hardly an egregious duplication for the _computer_
(something likely in a cache line != NULL) which aids readability and
reduces duplication for the _reader_ of the code for a path that is
inherently slow (likely going to fault in pages etc.)

I think it'd be confusing to have yet another split into
vma_can_track_dirty() or whatever because then suddenly for the check to be
meaningful you have to _always_ check 2 things.

Other options like passing an output parameter or returning something other
than boolean are equally distasteful.

>
> Also, with the exception of that double call to
> vm_ops_needs_writenotify(), these changes to mmap.c are code cleanup
> that has the same behavior as before. As such, it's better to separate
> them out from this patch whose goal is very much to change behavior.

It's not really cleanup, it's separating out some of the logic explicitly
to be used in this new context, without which the separation would not be
useful, so I feel it's a bit over the top to turn a small single patch into
two simply to avoid this.

>
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> thanks,
> --
> John Hubbard
> NVIDIA
>

Thanks for the review, I will respin with the suggestions (other than ones
I don't quite agree with as explained above) and a clearer description in
line with Mika's suggestions.

Hopefully we can move closer to this actually getting some
reviewed/acked-by tags soon :)