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