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

From: Lorenzo Stoakes
Date: Wed Apr 26 2023 - 03:01:23 EST

Next message: Ian Rogers: "[PATCH v1 00/40] Fix perf on Intel hybrid CPUs"
Previous message: Dikshita Agarwal: "[PATCH] venus: add support for 10 bit decoding."
In reply to: Mika Penttilä: "Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default"
Next in thread: Mika Penttilä: "Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Apr 26, 2023 at 06:18:38AM +0300, Mika Penttilä wrote:
> Hi,
>
>
> On 26.4.2023 2.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.
> >
> > 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.
> >
> > Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
> >
> > 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@xxxxxxxxx/
> >
> > v1:
> > https://lore.kernel.org/all/f86dc089b460c80805e321747b0898fd1efe93d7.1682168199.git.lstoakes@xxxxxxxxx/
> >
> > 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)
> > +{
> > + /* 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);
> > 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(
> > + 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);
> > +}
> > +
>
> What would be the exact reproducer of the problem? AFAIK writenotify is
> handled (by handle_mm_fault()) for non cow mappings (shared), where it only
> matters.

The issue is reproduced simply by page_to_virt(pinned_page)[0] = 'x' :)

The problem is that no faulting actually occurs, so no writenotify, and no
PG_dirty tracking does either. Unexpected page dirtying can occur even
after they are cleaned in folio_clear_dirty_for_io(), because the caller
might manually mark the page dirty at an unexpected time as with the
unpin_*dirty*() helpers.

I think the long-term solution is to provide a different interface where
pages are passed back briefly with locks held and with a manual invocation
of writeprotect, or perhaps some kthread_use_mm() thing so we actually
trigger the faulting logic, but in the meantime this change helps restore
some sanity.

>
> GUP will only allow FOLL_FORCE without faulting for PageAnonExclusive pages.
> So if you want something beyond normal cow semantics you have custom vm_ops
> (and mmap() and fault())

This has nothing to do with FOLL_FORCE.

>
> Also for longterm pinning gups vs fork vs swap there has been fixes by david
> recently.

I don't think these are relevant in any way to this issue.

>
>
>
> > /*
> > * 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))
> > 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);
> > }
> > /*
>
>
> --Mika
>

Next message: Ian Rogers: "[PATCH v1 00/40] Fix perf on Intel hybrid CPUs"
Previous message: Dikshita Agarwal: "[PATCH] venus: add support for 10 bit decoding."
In reply to: Mika Penttilä: "Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default"
Next in thread: Mika Penttilä: "Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings by default"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]