Message-ID: <5ffd7f32-d236-4da4-93f7-c2fe39a6e035@lucifer.local>
Date:   Wed, 26 Apr 2023 08:00:53 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     Mika Penttilä <mpenttil@...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>,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings
 by default

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@...dia.com>
> > Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
> > ---
> > 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)
> > +{
> > +	/* 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
>

Powered by blists - more mailing lists