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