Re: [PATCH] mm: perform all memfd seal checks in a single place

[SeongJae Park]

Hi Lorenzo,

On Fri, 6 Dec 2024 21:28:46 +0000 Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> wrote:

> We no longer actually need to perform these checks in the f_op->mmap() hook
> any longer.
> 
> We already moved the operation which clears VM_MAYWRITE on a read-only
> mapping of a write-sealed memfd in order to work around the restrictions
> imposed by commit 5de195060b2e ("mm: resolve faulty mmap_region() error
> path behaviour").
> 
> There is no reason for us not to simply go ahead and additionally check to
> see if any pre-existing seals are in place here rather than defer this to
> the f_op->mmap() hook.
> 
> By doing this we remove more logic from shmem_mmap() which doesn't belong
> there, as well as doing the same for hugetlbfs_file_mmap(). We also remove
> dubious shared logic in mm.h which simply does not belong there either.
> 
> It makes sense to do these checks at the earliest opportunity, we know
> these are shmem (or hugetlbfs) mappings whose relevant VMA flags will not
> change from the invoking do_mmap() so there is simply no need to wait.
> 
> This also means the implementation of further memfd seal flags can be done
> within mm/memfd.c and also have the opportunity to modify VMA flags as
> necessary early in the mapping logic.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
>  fs/hugetlbfs/inode.c  |  5 ----
>  include/linux/memfd.h | 22 ++++++++---------
>  include/linux/mm.h    | 55 -------------------------------------------
>  mm/memfd.c            | 44 +++++++++++++++++++++++++++++++++-
>  mm/mmap.c             | 12 +++++++---
>  mm/shmem.c            |  6 -----
>  6 files changed, 62 insertions(+), 82 deletions(-)
> 
[...]
> diff --git a/include/linux/memfd.h b/include/linux/memfd.h
> index d437e3070850..d53408b0bd31 100644
> --- a/include/linux/memfd.h
> +++ b/include/linux/memfd.h
> @@ -7,7 +7,14 @@
>  #ifdef CONFIG_MEMFD_CREATE
>  extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned int arg);
>  struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx);
> -unsigned int *memfd_file_seals_ptr(struct file *file);
> +/*
> + * Check for any existing seals on mmap, return an error if access is denied due
> + * to sealing, or 0 otherwise.
> + *
> + * We also update VMA flags if appropriate by manipulating the VMA flags pointed
> + * to by vm_flags_ptr.
> + */
> +int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr);
>  #else
>  static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned int a)
>  {
> @@ -17,19 +24,10 @@ static inline struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
>  {
>  	return ERR_PTR(-EINVAL);
>  }
> -
> -static inline unsigned int *memfd_file_seals_ptr(struct file *file)
> +int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags)

Shouldn't we set this function as 'static inline'?  Otherwise, build fails when
CONFIG_MEMFD_CREATE is not defined, like below.

    ld: mm/mmap.o: in function `memfd_check_seals_mmap':
    mmap.c:(.text+0x...): multiple definition of `memfd_check_seals_mmap'; mm/gup.o:gup.c:(.text+0x...): first defined here
    ld: mm/secretmem.o: in function `memfd_check_seals_mmap':
    secretmem.c:(.text+0x...): multiple definition of `memfd_check_seals_mmap'; mm/gup.o:gup.c:(.text+0x...): first defined here

Also a trivial nit.  The second argument's name (vm_flags) is different from
that for CONFIG_MEMFD_CREATE=y (vm_flags_ptr).

>  {
> -	return NULL;
> +	return 0;
>  }
>  #endif
>  
> -/* Retrieve memfd seals associated with the file, if any. */
> -static inline unsigned int memfd_file_seals(struct file *file)
> -{
> -	unsigned int *sealsp = memfd_file_seals_ptr(file);
> -
> -	return sealsp ? *sealsp : 0;
> -}
> -
>  #endif /* __LINUX_MEMFD_H */


Thanks,
SJ

[...]