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.
[1] https://android.googlesource.com/kernel/common/+/60500a42286de35f00d2a195f2021bcc029f11a1%5E%21/#F1
