Hi Colin,
On Fri, 21 Aug 2020 at 04:58, Colin Cross <ccross@xxxxxxxxxxx> wrote:
>
> On Thu, Aug 20, 2020 at 12:58 AM Michal Hocko <mhocko@xxxxxxxx> 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.
