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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpF3eSwW_Z48e0bykCh=8eohAuACxjXBbUV_sjrVwezxdw@mail.gmail.com>
Date: Mon, 22 Jul 2024 12:09:21 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Matthew Wilcox <willy@...radead.org>, "Paul E. McKenney" <paulmck@...nel.org>, 
	Andrii Nakryiko <andrii.nakryiko@...il.com>, Masami Hiramatsu <mhiramat@...nel.org>, mingo@...nel.org, 
	andrii@...nel.org, linux-kernel@...r.kernel.org, rostedt@...dmis.org, 
	oleg@...hat.com, jolsa@...nel.org, clm@...a.com, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 00/10] perf/uprobe: Optimize uprobes

On Wed, Jul 10, 2024 at 2:40 AM Peter Zijlstra <peterz@...radead.org> 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@mail.gmail.com/).
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.

> +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);
>