Re: [PATCH v2 2/4] mm, madvise: extract mm code from prctl_set_vma() to mm/madvise.c

[Suren Baghdasaryan <surenb@google.com>]

On Tue, Jun 24, 2025 at 7:04 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 24.06.25 15:03, Vlastimil Babka wrote:
> > Setting anon_name is done via madvise_set_anon_name() and behaves a lot
> > of like other madvise operations. However, apparently because madvise()
> > has lacked the 4th argument and prctl() not, the userspace entry point
> > has been implemented via prctl(PR_SET_VMA, ...) and handled first by
> > prctl_set_vma().
> >
> > Currently prctl_set_vma() lives in kernel/sys.c but setting the
> > vma->anon_name is mm-specific code so extract it to a new
> > set_anon_vma_name() function under mm. mm/madvise.c seems to be the most
> > straightforward place as that's where madvise_set_anon_name() lives.
> > Stop declaring the latter in mm.h and instead declare
> > set_anon_vma_name().
> >
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> > ---
> >   include/linux/mm.h | 14 +++++++-------
> >   kernel/sys.c       | 50 +-------------------------------------------------
> >   mm/madvise.c       | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   3 files changed, 58 insertions(+), 58 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 0e0549f3d681f6c7a78e8dfa341a810e5a8f96c1..ef40f68c1183d4c95016575a4ee0171e12df9ba4 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4059,14 +4059,14 @@ unsigned long wp_shared_mapping_range(struct address_space *mapping,
> >   #endif
> >
> >   #ifdef CONFIG_ANON_VMA_NAME
> > -int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > -                       unsigned long len_in,
> > -                       struct anon_vma_name *anon_name);
> > +int set_anon_vma_name(unsigned long addr, unsigned long size,
> > +                   const char __user *uname);
> >   #else
> > -static inline int
> > -madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > -                   unsigned long len_in, struct anon_vma_name *anon_name) {
> > -     return 0;
> > +static inline
> > +int set_anon_vma_name(unsigned long addr, unsigned long size,
> > +                   const char __user *uname)
> > +{
> > +     return -EINVAL;
> >   }
> >   #endif
> >
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index adc0de0aa364aebb23999f621717a5d32599921c..b153fb345ada28ea1a33386a32bcce9cb1b23475 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2343,54 +2343,14 @@ int __weak arch_lock_shadow_stack_status(struct task_struct *t, unsigned long st
> >
> >   #define PR_IO_FLUSHER (PF_MEMALLOC_NOIO | PF_LOCAL_THROTTLE)
> >
> > -#ifdef CONFIG_ANON_VMA_NAME
> > -
> > -#define ANON_VMA_NAME_MAX_LEN                80
> > -#define ANON_VMA_NAME_INVALID_CHARS  "\\`$[]"
> > -
> > -static inline bool is_valid_name_char(char ch)
> > -{
> > -     /* printable ascii characters, excluding ANON_VMA_NAME_INVALID_CHARS */
> > -     return ch > 0x1f && ch < 0x7f &&
> > -             !strchr(ANON_VMA_NAME_INVALID_CHARS, ch);
> > -}
> > -
> >   static int prctl_set_vma(unsigned long opt, unsigned long addr,
> >                        unsigned long size, unsigned long arg)
> >   {
> > -     struct mm_struct *mm = current->mm;
> > -     const char __user *uname;
> > -     struct anon_vma_name *anon_name = NULL;
> >       int error;
> >
> >       switch (opt) {
> >       case PR_SET_VMA_ANON_NAME:
> > -             uname = (const char __user *)arg;
> > -             if (uname) {
> > -                     char *name, *pch;
> > -
> > -                     name = strndup_user(uname, ANON_VMA_NAME_MAX_LEN);
> > -                     if (IS_ERR(name))
> > -                             return PTR_ERR(name);
> > -
> > -                     for (pch = name; *pch != '\0'; pch++) {
> > -                             if (!is_valid_name_char(*pch)) {
> > -                                     kfree(name);
> > -                                     return -EINVAL;
> > -                             }
> > -                     }
> > -                     /* anon_vma has its own copy */
> > -                     anon_name = anon_vma_name_alloc(name);
> > -                     kfree(name);
> > -                     if (!anon_name)
> > -                             return -ENOMEM;
> > -
> > -             }
> > -
> > -             mmap_write_lock(mm);
> > -             error = madvise_set_anon_name(mm, addr, size, anon_name);
> > -             mmap_write_unlock(mm);
> > -             anon_vma_name_put(anon_name);
> > +             error = set_anon_vma_name(addr, size, (const char __user *)arg);
>
> At first I thought whether passing current->mm as an argument might make
> it clearer on what we actually operate. But then, "anon_vma" might give
> a good hint.
>
> Acked-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

>
> --
> Cheers,
>
> David / dhildenb
>