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

[Andrii Nakryiko]

On Mon, Jul 22, 2024 at 12:09 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Wed, Jul 10, 2024 at 2:40 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, Jul 10, 2024 at 11:16:31AM +0200, Peter Zijlstra wrote:
> >
> > > If it were an actual sequence count, I could make it work, but sadly,
> > > not. Also, vma_end_write() seems to be missing :-( If anything it could
> > > be used to lockdep annotate the thing.
>
> Thanks Matthew for forwarding me this discussion!
>
> > >
> > > Mooo.. I need to stare more at this to see if perhaps it can be made to
> > > work, but so far, no joy :/
> >
> > See, this is what I want, except I can't close the race against VMA
> > modification because of that crazy locking scheme :/
>
> Happy to explain more about this crazy locking scheme. The catch is
> that we can write-lock a VMA only while holding mmap_lock for write
> and we unlock all write-locked VMAs together when we drop that
> mmap_lock:
>
> mmap_write_lock(mm);
> vma_start_write(vma1);
> vma_start_write(vma2);
> ...
> mmap_write_unlock(mm); -> vma_end_write_all(mm); // unlocks all locked vmas
>
> This is done because oftentimes we need to lock multiple VMAs when
> modifying the address space (vma merge/split) and unlocking them
> individually would be more expensive than unlocking them in bulk by
> incrementing mm->mm_lock_seq.
>
> >
> >
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2146,11 +2146,58 @@ static int is_trap_at_addr(struct mm_str
> >         return is_trap_insn(&opcode);
> >  }
> >
> > -static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> > +#ifndef CONFIG_PER_VMA_LOCK
> > +static struct uprobe *__find_active_uprobe(unsigned long bp_vaddr)
> > +{
> > +       return NULL;
> > +}
> > +#else
>
> IIUC your code below, you want to get vma->vm_file without locking the
> VMA. I think under RCU that would have been possible if vma->vm_file
> were RCU-safe, which it's not (we had discussions with Paul and
> Matthew about that in
> https://lore.kernel.org/all/CAJuCfpHW2=Zu+CHXL+5fjWxGk=CVix=C66ra+DmXgn6r3+fsXg@xxxxxxxxxxxxxx/).
> Otherwise you could store the value of vma->vm_lock_seq before
> comparing it with mm->mm_lock_seq, then do get_file(vma->file) and
> then compare your locally stored vm_lock_seq against vma->vm_lock_seq
> to see if VMA got locked for modification after we got the file. So,
> unless I miss some other race, I think the VMA locking sequence does
> not preclude you from implementing __find_active_uprobe() but
> accessing vma->vm_file would be unsafe without some kind of locking.

Hey Suren!

I've haven't yet dug properly into this, but from quick checking
around I think for the hot path (where this all matters), we really
only want to get vma's underlying inode. vm_file itself is just a
means to that end. If there is some clever way to do
vma->vm_file->f_inode under RCU and without mmap_read_lock, that would
be good enough, I think.

>
> > +static struct uprobe *__find_active_uprobe(unsigned long bp_vaddr)
> >  {
> >         struct mm_struct *mm = current->mm;
> >         struct uprobe *uprobe = NULL;
> >         struct vm_area_struct *vma;
> > +       MA_STATE(mas, &mm->mm_mt, bp_vaddr, bp_vaddr);
> > +
> > +       guard(rcu)();
> > +
> > +again:
> > +       vma = mas_walk(&mas);
> > +       if (!vma)
> > +               return NULL;
> > +
> > +       /* vma_write_start() -- in progress */
> > +       if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(vma->vm_mm->mm_lock_seq))
> > +               return NULL;
> > +
> > +       /*
> > +        * Completely broken, because of the crazy vma locking scheme you
> > +        * cannot avoid the per-vma rwlock and doing so means you're racy
> > +        * against modifications.
> > +        *
> > +        * A simple actual seqcount would'be been cheaper and more usefull.
> > +        */
> > +
> > +       if (!valid_vma(vma, false))
> > +               return NULL;
> > +
> > +       struct inode = file_inode(vma->vm_file);
> > +       loff_t offset = vaddr_to_offset(vma, bp_vaddr);
> > +
> > +       // XXX: if (vma_seq_retry(...)) goto again;
> > +
> > +       return find_uprobe(inode, offset);
> > +}
> > +#endif
> > +
> > +static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> > +{
> > +       struct uprobe *uprobe = __find_active_uprobe(bp_vaddr)
> > +       struct mm_struct *mm = current->mm;
> > +       struct vm_area_struct *vma;
> > +
> > +       if (uprobe)
> > +               return uprobe;
> >
> >         mmap_read_lock(mm);
> >         vma = vma_lookup(mm, bp_vaddr);
> >