Re: [PATCH v13 19/26] mm: Re-introduce do_mmap_pgoff()

[Peter Collingbourne <pcc@google.com>]

On Fri, Oct 2, 2020 at 8:58 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote:
>
> On 10/1/2020 7:06 PM, Peter Collingbourne wrote:
> > On Fri, Sep 25, 2020 at 7:57 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >>
> >> There was no more caller passing vm_flags to do_mmap(), and vm_flags was
> >> removed from the function's input by:
> >>
> >>      commit 45e55300f114 ("mm: remove unnecessary wrapper function do_mmap_pgoff()").
> >>
> >> There is a new user now.  Shadow stack allocation passes VM_SHSTK to
> >> do_mmap().  Re-introduce the vm_flags and do_mmap_pgoff().
> >
> > I would prefer to change the callers to pass the additional 0 argument
> > instead of bringing the wrapper function back, but if we're going to
> > bring it back then we should fix the naming (both functions take a
> > pgoff argument, so the previous name do_mmap_pgoff() was just plain
> > confusing).
> >
> > Peter
> >
>
> Thanks for your feedback.  Here is the updated patch.  I will re-send
> the whole series later.
>
> Yu-cheng
>
> ======
>
>  From 6a9f1e6bcdb6e599a44d5f58cf4cebd28c4634a2 Mon Sep 17 00:00:00 2001
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Date: Wed, 12 Aug 2020 14:01:58 -0700
> Subject: [PATCH 19/26] mm: Re-introduce do_mmap_pgoff()

The subject line of the commit message needs to be updated, but aside from that:

Reviewed-by: Peter Collingbourne <pcc@google.com>

Peter

>
> There was no more caller passing vm_flags to do_mmap(), and vm_flags was
> removed from the function's input by:
>
>      commit 45e55300f114 ("mm: remove unnecessary wrapper function
> do_mmap_pgoff()").
>
> There is a new user now.  Shadow stack allocation passes VM_SHSTK to
> do_mmap().  Re-introduce vm_flags to do_mmap(), but without the old wrapper
> do_mmap_pgoff().  Instead, fix all callers of the wrapper by passing a zero
> vm_flags to do_mmap().
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Cc: Peter Collingbourne <pcc@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: linux-mm@kvack.org
> ---
>   fs/aio.c           |  2 +-
>   include/linux/mm.h |  3 ++-
>   ipc/shm.c          |  2 +-
>   mm/mmap.c          | 10 +++++-----
>   mm/nommu.c         |  4 ++--
>   mm/util.c          |  2 +-
>   6 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index d5ec30385566..ca8c11665eea 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -527,7 +527,7 @@ static int aio_setup_ring(struct kioctx *ctx,
> unsigned int nr_events)
>
>         ctx->mmap_base = do_mmap(ctx->aio_ring_file, 0, ctx->mmap_size,
>                                  PROT_READ | PROT_WRITE,
> -                                MAP_SHARED, 0, &unused, NULL);
> +                                MAP_SHARED, 0, 0, &unused, NULL);
>         mmap_write_unlock(mm);
>         if (IS_ERR((void *)ctx->mmap_base)) {
>                 ctx->mmap_size = 0;
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e09d13699bbe..e020eea33138 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2560,7 +2560,8 @@ extern unsigned long mmap_region(struct file
> *file, unsigned long addr,
>         struct list_head *uf);
>   extern unsigned long do_mmap(struct file *file, unsigned long addr,
>         unsigned long len, unsigned long prot, unsigned long flags,
> -       unsigned long pgoff, unsigned long *populate, struct list_head *uf);
> +       vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate,
> +       struct list_head *uf);
>   extern int __do_munmap(struct mm_struct *, unsigned long, size_t,
>                        struct list_head *uf, bool downgrade);
>   extern int do_munmap(struct mm_struct *, unsigned long, size_t,
> diff --git a/ipc/shm.c b/ipc/shm.c
> index e25c7c6106bc..91474258933d 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1556,7 +1556,7 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg,
>                         goto invalid;
>         }
>
> -       addr = do_mmap(file, addr, size, prot, flags, 0, &populate, NULL);
> +       addr = do_mmap(file, addr, size, prot, flags, 0, 0, &populate, NULL);
>         *raddr = addr;
>         err = 0;
>         if (IS_ERR_VALUE(addr))
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 574b3f273462..fc04184d2eae 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1365,11 +1365,11 @@ static inline bool file_mmap_ok(struct file
> *file, struct inode *inode,
>    */
>   unsigned long do_mmap(struct file *file, unsigned long addr,
>                         unsigned long len, unsigned long prot,
> -                       unsigned long flags, unsigned long pgoff,
> -                       unsigned long *populate, struct list_head *uf)
> +                       unsigned long flags, vm_flags_t vm_flags,
> +                       unsigned long pgoff, unsigned long *populate,
> +                       struct list_head *uf)
>   {
>         struct mm_struct *mm = current->mm;
> -       vm_flags_t vm_flags;
>         int pkey = 0;
>
>         *populate = 0;
> @@ -1431,7 +1431,7 @@ unsigned long do_mmap(struct file *file, unsigned
> long addr,
>          * to. we assume access permissions have been handled by the open
>          * of the memory object, so we don't do any here.
>          */
> -       vm_flags = calc_vm_prot_bits(prot, pkey) | calc_vm_flag_bits(flags) |
> +       vm_flags |= calc_vm_prot_bits(prot, pkey) | calc_vm_flag_bits(flags) |
>                         mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
>
>         if (flags & MAP_LOCKED)
> @@ -3007,7 +3007,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long,
> start, unsigned long, size,
>
>         file = get_file(vma->vm_file);
>         ret = do_mmap(vma->vm_file, start, size,
> -                       prot, flags, pgoff, &populate, NULL);
> +                       prot, flags, 0, pgoff, &populate, NULL);
>         fput(file);
>   out:
>         mmap_write_unlock(mm);
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 75a327149af1..f67d6bcdfc9f 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1078,6 +1078,7 @@ unsigned long do_mmap(struct file *file,
>                         unsigned long len,
>                         unsigned long prot,
>                         unsigned long flags,
> +                       vm_flags_t vm_flags,
>                         unsigned long pgoff,
>                         unsigned long *populate,
>                         struct list_head *uf)
> @@ -1085,7 +1086,6 @@ unsigned long do_mmap(struct file *file,
>         struct vm_area_struct *vma;
>         struct vm_region *region;
>         struct rb_node *rb;
> -       vm_flags_t vm_flags;
>         unsigned long capabilities, result;
>         int ret;
>
> @@ -1104,7 +1104,7 @@ unsigned long do_mmap(struct file *file,
>
>         /* we've determined that we can make the mapping, now translate what we
>          * now know into VMA flags */
> -       vm_flags = determine_vm_flags(file, prot, flags, capabilities);
> +       vm_flags |= determine_vm_flags(file, prot, flags, capabilities);
>
>         /* we're going to need to record the mapping */
>         region = kmem_cache_zalloc(vm_region_jar, GFP_KERNEL);
> diff --git a/mm/util.c b/mm/util.c
> index 5ef378a2a038..beb8b881c080 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -503,7 +503,7 @@ unsigned long vm_mmap_pgoff(struct file *file,
> unsigned long addr,
>         if (!ret) {
>                 if (mmap_write_lock_killable(mm))
>                         return -EINTR;
> -               ret = do_mmap(file, addr, len, prot, flag, pgoff, &populate,
> +               ret = do_mmap(file, addr, len, prot, flag, 0, pgoff, &populate,
>                               &uf);
>                 mmap_write_unlock(mm);
>                 userfaultfd_unmap_complete(mm, &uf);
> --
> 2.21.0
>