Re: [PATCH 00/10] perf/uprobe: Optimize uprobes

[Suren Baghdasaryan]

On Tue, Jul 30, 2024 at 6:11 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Sat, Jul 27, 2024 at 04:45:53AM +0100, Matthew Wilcox wrote:
>
> > Hum.  What if we added SLAB_TYPESAFE_BY_RCU to files_cachep?  That way
> > we could do:
> >
> >       inode = NULL;
> >       rcu_read_lock();
> >       vma = find_vma(mm, address);
> >       if (!vma)
> >               goto unlock;
> >       file = READ_ONCE(vma->vm_file);
> >       if (!file)
> >               goto unlock;
> >       inode = file->f_inode;
> >       if (file != READ_ONCE(vma->vm_file))
> >               inode = NULL;
>
> remove_vma() does not clear vm_file, nor do I think we ever re-assign
> this field after it is set on creation.

Quite correct and even if we clear vm_file in remove_vma() and/or
reset it on creation I don't think that would be enough. IIUC the
warning about SLAB_TYPESAFE_BY_RCU here:
https://elixir.bootlin.com/linux/v6.10.2/source/include/linux/slab.h#L98
means that the vma object can be reused in the same RCU grace period.

>
> That is, I'm struggling to see what this would do. AFAICT this can still
> happen:
>
>         rcu_read_lock();
>         vma = find_vma();
>                                         remove_vma()
>                                           fput(vma->vm_file);
>                                                                 dup_fd)
>                                                                   newf = kmem_cache_alloc(...)
>                                                                   newf->f_inode = blah
>

Imagine that the vma got freed and reused at this point. Then
vma->vm_file might be pointing to a valid but a completely different
file.

>         file = READ_ONCE(vma->vm_file);
>         inode = file->f_inode; // blah
>         if (file != READ_ONCE(vma->vm_file)) // still match

I think we should also check that the VMA represents the same area
after we obtained the inode.

>
>
> > unlock:
> >       rcu_read_unlock();
> >
> >       if (inode)
> >               return inode;
> >       mmap_read_lock();
> >       vma = find_vma(mm, address);
> >       ...
> >
> > I think this would be safe because 'vma' will not be reused while we
> > hold the read lock, and while 'file' might be reused, whatever f_inode
> > points to won't be used if vm_file is no longer what it once was.
>
>
> Also, we need vaddr_to_offset() which needs additional serialization
> against vma_lock.