On Fri, Jul 26, 2024 at 8:45 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Fri, Jul 26, 2024 at 06:29:44PM -0700, Suren Baghdasaryan wrote:
> > On Fri, Jul 26, 2024 at 5:20 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > 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.
> >
> > Hi Andrii,
> > Sorry, I'm not aware of any other way to get the inode from vma. Maybe
> > Matthew with his FS background can find a way?
>
> 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;
> 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.
>
> On the other hand, it's quarter to midnight on Friday, and I have a
> terrible virus that I'm struggling through, so not ideal circumstances
> for me to be reasoning about RCU guarantees.
Hi Matthew,
Hopefully you got some rest over the weekend and are feeling better!
What you proposed above with SLAB_TYPESAFE_BY_RCU (assuming it is safe
and correct) I think would solve the uprobe scalability problem when
it comes to mmap_lock. So did you get a chance to think this through
some more? And if that still seems correct, how should we go about
making this happen?
