Re: [PATCH v5 2/2] mm: add a field to store names for private anonymous memory

[Sumit Semwal <sumit.semwal@linaro.org>]

Hi Colin,

On Fri, 21 Aug 2020 at 04:58, Colin Cross <ccross@android.com> wrote:
>
> On Thu, Aug 20, 2020 at 12:58 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 19-08-20 19:46:50, Sumit Semwal wrote:
> > [...]
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index 5066b0251ed8..136fd3c3ad7b 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -97,6 +97,21 @@ unsigned long task_statm(struct mm_struct *mm,
> > >       return mm->total_vm;
> > >  }
> > >
> > > +static void seq_print_vma_name(struct seq_file *m, struct vm_area_struct *vma)
> > > +{
> > > +     struct mm_struct *mm = vma->vm_mm;
> > > +     char anon_name[NAME_MAX + 1];
> > > +     int n;
> > > +
> > > +     n = access_remote_vm(mm, (unsigned long)vma_anon_name(vma),
> > > +                          anon_name, NAME_MAX, 0);
> > > +     if (n > 0) {
> > > +             seq_puts(m, "[anon:");
> > > +             seq_write(m, anon_name, strnlen(anon_name, n));
> > > +             seq_putc(m, ']');
> > > +     }
> > > +}
> > > +
> > >  #ifdef CONFIG_NUMA
> > >  /*
> > >   * Save get_task_policy() for show_numa_map().
> > > @@ -319,8 +334,15 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
> > >                       goto done;
> > >               }
> > >
> > > -             if (is_stack(vma))
> > > +             if (is_stack(vma)) {
> > >                       name = "[stack]";
> > > +                     goto done;
> > > +             }
> > > +
> > > +             if (vma_anon_name(vma)) {
> > > +                     seq_pad(m, ' ');
> > > +                     seq_print_vma_name(m, vma);
> > > +             }
> > >       }
> >
> > How can be this safe? access_remote_vm requires mmap_sem (non exlusive).
> > The same is the case for show_map_vma. So what would happen if a task
> > sets its own name? IIRC semaphore code doesn't allow read lock nesting
> > because any exclusive lock request in the mean time would block further
> > readers. Or is this allowed?
>
> Good catch.  The version of this patch that has been in use the
> Android kernel since 2015 [1] doesn't have this issue because it
> doesn't use access_remote_vm, it calls get_user_pages_remote directly.
> This would need to call a version of access_remote_vm that assumes the
> mmap_sem is already held.

Indeed. so does it sound ok to add an access_remote_vma_mmap_lockheld() version?

>
> [1] https://android.googlesource.com/kernel/common/+/60500a42286de35f00d2a195f2021bcc029f11a1%5E%21/#F1

Best,
Sumit.