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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Sat, Dec 07, 2024 at 11:22:51AM -0800, SeongJae Park wrote:
> Hi Lorenzo,
>
> On Fri, 6 Dec 2024 21:28:46 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> 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@oracle.com>
> > ---
> >  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.

Yep thanks, I typo'd this, will send a fix-patch.

>
>     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
>
> [...]