Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution

[Jann Horn]

On Mon, Sep 9, 2024 at 11:29 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
> On Mon, Sep 9, 2024 at 6:13 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> > On Fri, Sep 6, 2024 at 7:12 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > > +static struct uprobe *find_active_uprobe_speculative(unsigned long bp_vaddr)
> > > +{
> > > +       const vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_MAYSHARE;
> > > +       struct mm_struct *mm = current->mm;
> > > +       struct uprobe *uprobe;
> > > +       struct vm_area_struct *vma;
> > > +       struct file *vm_file;
> > > +       struct inode *vm_inode;
> > > +       unsigned long vm_pgoff, vm_start;
> > > +       int seq;
> > > +       loff_t offset;
> > > +
> > > +       if (!mmap_lock_speculation_start(mm, &seq))
> > > +               return NULL;
> > > +
> > > +       rcu_read_lock();
> > > +
> > > +       vma = vma_lookup(mm, bp_vaddr);
> > > +       if (!vma)
> > > +               goto bail;
> > > +
> > > +       vm_file = data_race(vma->vm_file);
> >
> > A plain "data_race()" says "I'm fine with this load tearing", but
> > you're relying on this load not tearing (since you access the vm_file
> > pointer below).
> > You're also relying on the "struct file" that vma->vm_file points to
> > being populated at this point, which means you need CONSUME semantics
> > here, which READ_ONCE() will give you, and something like RELEASE
> > semantics on any pairing store that populates vma->vm_file, which
> > means they'd all have to become something like smp_store_release()).
>
> vma->vm_file should be set in VMA before it is installed and is never
> modified afterwards, isn't that the case? So maybe no extra barrier
> are needed and READ_ONCE() would be enough.

Ah, right, I'm not sure what I was thinking there.

I... guess you only _really_ need the READ_ONCE() if something can
actually ever change the ->vm_file pointer, otherwise just a plain
load with no annotation whatsoever would be good enough? I'm fairly
sure nothing can ever change the ->vm_file pointer of a live VMA, and
I think _currently_ it looks like nothing will NULL out the ->vm_file
pointer on free either... though that last part is probably not
something you should rely on...