On Fri 21-08-20 08:51:57, Sumit Semwal wrote:
> 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?
You will still need to take the lock if the pointer belongs to a remote
address space. But how are you going to find out?
--
Michal Hocko
SUSE Labs
