uprobes: speculative lockless VMA-to-uprobe lookup

[PATCH v5 tip/perf/core 1/2] uprobes: simplify find_active_uprobe_rcu() VMA checks

Posted by Andrii Nakryiko 1 year, 2 months ago

At the point where find_active_uprobe_rcu() is used we know that VMA in
question has triggered software breakpoint, so we don't need to validate
vma->vm_flags. Keep only vma->vm_file NULL check.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/events/uprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a76ddc5fc982..c4da8f741f3a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2305,7 +2305,7 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
 	mmap_read_lock(mm);
 	vma = vma_lookup(mm, bp_vaddr);
 	if (vma) {
-		if (valid_vma(vma, false)) {
+		if (vma->vm_file) {
 			struct inode *inode = file_inode(vma->vm_file);
 			loff_t offset = vaddr_to_offset(vma, bp_vaddr);
 
-- 
2.43.5

Re: [PATCH v5 tip/perf/core 1/2] uprobes: simplify find_active_uprobe_rcu() VMA checks

Posted by Jann Horn 1 year, 2 months ago

On Fri, Nov 22, 2024 at 4:59 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> At the point where find_active_uprobe_rcu() is used we know that VMA in
> question has triggered software breakpoint, so we don't need to validate
> vma->vm_flags. Keep only vma->vm_file NULL check.

How do we know that the VMA we find triggered a software breakpoint?
Between the time a software breakpoint was hit and the time we took
the mmap_read_lock(), the VMA could have been replaced with an
entirely different one, right?

I don't know this code well, and your change looks like it's probably
fine (since the file is just used to look up its inode in some tree,
and therefore for incompatible files, the lookup is guaranteed to fail
and nothing will happen). But I think the commit message looks dodgy.

> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  kernel/events/uprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a76ddc5fc982..c4da8f741f3a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2305,7 +2305,7 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
>         mmap_read_lock(mm);
>         vma = vma_lookup(mm, bp_vaddr);
>         if (vma) {
> -               if (valid_vma(vma, false)) {
> +               if (vma->vm_file) {
>                         struct inode *inode = file_inode(vma->vm_file);
>                         loff_t offset = vaddr_to_offset(vma, bp_vaddr);
>
> --
> 2.43.5
>

Re: [PATCH v5 tip/perf/core 1/2] uprobes: simplify find_active_uprobe_rcu() VMA checks

Posted by Oleg Nesterov 1 year, 2 months ago

On 11/26, Jann Horn wrote:
>
> On Fri, Nov 22, 2024 at 4:59 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> > At the point where find_active_uprobe_rcu() is used we know that VMA in
> > question has triggered software breakpoint, so we don't need to validate
> > vma->vm_flags. Keep only vma->vm_file NULL check.
>
> How do we know that the VMA we find triggered a software breakpoint?
> Between the time a software breakpoint was hit and the time we took
> the mmap_read_lock(), the VMA could have been replaced with an
> entirely different one, right?

Right, but this doesn't really differ from the case when another thread
replaces (or even unmaps) this VMA after find_active_uprobe_rcu() drops
mm->mmap_lock and returns a found uprobe.

So I think this is fine.

Oleg.

Re: [PATCH v5 tip/perf/core 1/2] uprobes: simplify find_active_uprobe_rcu() VMA checks

Posted by Andrii Nakryiko 1 year, 2 months ago

On Tue, Nov 26, 2024 at 2:20 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Nov 22, 2024 at 4:59 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> > At the point where find_active_uprobe_rcu() is used we know that VMA in
> > question has triggered software breakpoint, so we don't need to validate
> > vma->vm_flags. Keep only vma->vm_file NULL check.
>
> How do we know that the VMA we find triggered a software breakpoint?
> Between the time a software breakpoint was hit and the time we took
> the mmap_read_lock(), the VMA could have been replaced with an
> entirely different one, right?

We need that VMA only to get inode + file offset, and whether it is
the original VMA with uprobe installed, or someone raced and replaced
it with some other VMA shouldn't matter. We either find uprobe at that
offset within that inode, or not. So this seems fine.

>
> I don't know this code well, and your change looks like it's probably
> fine (since the file is just used to look up its inode in some tree,
> and therefore for incompatible files, the lookup is guaranteed to fail
> and nothing will happen). But I think the commit message looks dodgy.
>
> > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > Suggested-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  kernel/events/uprobes.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index a76ddc5fc982..c4da8f741f3a 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2305,7 +2305,7 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
> >         mmap_read_lock(mm);
> >         vma = vma_lookup(mm, bp_vaddr);
> >         if (vma) {
> > -               if (valid_vma(vma, false)) {
> > +               if (vma->vm_file) {
> >                         struct inode *inode = file_inode(vma->vm_file);
> >                         loff_t offset = vaddr_to_offset(vma, bp_vaddr);
> >
> > --
> > 2.43.5
> >

[tip: perf/core] uprobes: simplify find_active_uprobe_rcu() VMA checks

Posted by tip-bot2 for Andrii Nakryiko 1 year, 2 months ago

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     83e3dc9a5d4d7402adb24090a77327245d593129
Gitweb:        https://git.kernel.org/tip/83e3dc9a5d4d7402adb24090a77327245d593129
Author:        Andrii Nakryiko <andrii@kernel.org>
AuthorDate:    Thu, 21 Nov 2024 19:59:21 -08:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 02 Dec 2024 12:01:38 +01:00

uprobes: simplify find_active_uprobe_rcu() VMA checks

At the point where find_active_uprobe_rcu() is used we know that VMA in
question has triggered software breakpoint, so we don't need to validate
vma->vm_flags. Keep only vma->vm_file NULL check.

Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lkml.kernel.org/r/20241122035922.3321100-2-andrii@kernel.org
---
 kernel/events/uprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index fa04b14..62c14df 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2304,7 +2304,7 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
 	mmap_read_lock(mm);
 	vma = vma_lookup(mm, bp_vaddr);
 	if (vma) {
-		if (valid_vma(vma, false)) {
+		if (vma->vm_file) {
 			struct inode *inode = file_inode(vma->vm_file);
 			loff_t offset = vaddr_to_offset(vma, bp_vaddr);