Message-ID: <7a3ff186-09c4-1059-9cdf-9e793f985251@nvidia.com>
Date:   Wed, 26 Apr 2023 17:52:09 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
CC:     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>, Jan Kara <jack@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Pavel Begunkov <asml.silence@...il.com>
Subject: Re: [PATCH v4] mm/gup: disallow GUP writing to file-backed mappings
 by default

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.


> 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 <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)

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".

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.

> +{
> +	/* 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.

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.

> +	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.

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.


[1] https://lore.kernel.org/all/1b9e3406-c08e-b97c-d46f-22f36535d9e5@nvidia.com/

thanks,
-- 
John Hubbard
NVIDIA

Powered by blists - more mailing lists