If uprobe consumer changes instruction pointer we still execute
(single step or emulate) the original instruction and increment
the ip register with the size of the instruction.
In case the instruction is emulated, the new ip register value is
incremented with the instructions size and process is likely to
crash with illegal instruction.
In case the instruction is single-stepped, the ip register change
is lost and process continues with the original ip register value.
If user decided to take execution elsewhere, it makes little sense
to execute the original instruction, so let's skip it. Allowing this
behaviour only for uprobe with unique consumer attached.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/events/uprobes.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b9b088f7333a..da8291941c6b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2568,7 +2568,7 @@ static bool ignore_ret_handler(int rc)
return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
}
-static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
+static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
{
struct uprobe_consumer *uc;
bool has_consumers = false, remove = true;
@@ -2582,6 +2582,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
__u64 cookie = 0;
int rc = 0;
+ if (is_unique)
+ *is_unique |= uc->is_unique;
+
if (uc->handler) {
rc = uc->handler(uc, regs, &cookie);
WARN(rc < 0 || rc > 2,
@@ -2735,6 +2738,7 @@ static void handle_swbp(struct pt_regs *regs)
{
struct uprobe *uprobe;
unsigned long bp_vaddr;
+ bool is_unique = false;
int is_swbp;
bp_vaddr = uprobe_get_swbp_addr(regs);
@@ -2789,7 +2793,10 @@ static void handle_swbp(struct pt_regs *regs)
if (arch_uprobe_ignore(&uprobe->arch, regs))
goto out;
- handler_chain(uprobe, regs);
+ handler_chain(uprobe, regs, &is_unique);
+
+ if (is_unique && instruction_pointer(regs) != bp_vaddr)
+ goto out;
/* Try to optimize after first hit. */
arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
@@ -2819,7 +2826,7 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
return;
if (arch_uprobe_ignore(&uprobe->arch, regs))
return;
- handler_chain(uprobe, regs);
+ handler_chain(uprobe, regs, NULL);
}
/*
--
2.51.0On 09/02, Jiri Olsa wrote:
>
> If user decided to take execution elsewhere, it makes little sense
> to execute the original instruction, so let's skip it.
Exactly.
So why do we need all these "is_unique" complications? Only a single
is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp()
can just do
handler_chain(uprobe, regs);
if (instruction_pointer(regs) != bp_vaddr)
goto out;
> Allowing this
> behaviour only for uprobe with unique consumer attached.
But if a non-exclusive consumer changes regs->ip, we have a problem
anyway, right?
We can probably add something like
rc = uc->handler(uc, regs, &cookie);
+ WARN_ON(!uc->is_unique && instruction_pointer(regs) != bp_vaddr);
into handler_chain(), although I don't think this is needed.
Oleg.
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/events/uprobes.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b9b088f7333a..da8291941c6b 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2568,7 +2568,7 @@ static bool ignore_ret_handler(int rc)
> return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
> }
>
> -static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> +static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
> {
> struct uprobe_consumer *uc;
> bool has_consumers = false, remove = true;
> @@ -2582,6 +2582,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> __u64 cookie = 0;
> int rc = 0;
>
> + if (is_unique)
> + *is_unique |= uc->is_unique;
> +
> if (uc->handler) {
> rc = uc->handler(uc, regs, &cookie);
> WARN(rc < 0 || rc > 2,
> @@ -2735,6 +2738,7 @@ static void handle_swbp(struct pt_regs *regs)
> {
> struct uprobe *uprobe;
> unsigned long bp_vaddr;
> + bool is_unique = false;
> int is_swbp;
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> @@ -2789,7 +2793,10 @@ static void handle_swbp(struct pt_regs *regs)
> if (arch_uprobe_ignore(&uprobe->arch, regs))
> goto out;
>
> - handler_chain(uprobe, regs);
> + handler_chain(uprobe, regs, &is_unique);
> +
> + if (is_unique && instruction_pointer(regs) != bp_vaddr)
> + goto out;
>
> /* Try to optimize after first hit. */
> arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
> @@ -2819,7 +2826,7 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
> return;
> if (arch_uprobe_ignore(&uprobe->arch, regs))
> return;
> - handler_chain(uprobe, regs);
> + handler_chain(uprobe, regs, NULL);
> }
>
> /*
> --
> 2.51.0
>
On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote:
> On 09/02, Jiri Olsa wrote:
> >
> > If user decided to take execution elsewhere, it makes little sense
> > to execute the original instruction, so let's skip it.
>
> Exactly.
>
> So why do we need all these "is_unique" complications? Only a single
> is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp()
> can just do
>
> handler_chain(uprobe, regs);
> if (instruction_pointer(regs) != bp_vaddr)
> goto out;
hum, that's what I did in rfc [1] but I thought you did not like that [2]
[1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/
[2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/
I guess I misunderstood your reply [2], I'd be happy to drop the
unique/exclusive flag
jirka
>
>
> > Allowing this
> > behaviour only for uprobe with unique consumer attached.
>
> But if a non-exclusive consumer changes regs->ip, we have a problem
> anyway, right?
>
> We can probably add something like
>
> rc = uc->handler(uc, regs, &cookie);
> + WARN_ON(!uc->is_unique && instruction_pointer(regs) != bp_vaddr);
>
> into handler_chain(), although I don't think this is needed.
>
> Oleg.
>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/events/uprobes.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index b9b088f7333a..da8291941c6b 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2568,7 +2568,7 @@ static bool ignore_ret_handler(int rc)
> > return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
> > }
> >
> > -static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > +static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
> > {
> > struct uprobe_consumer *uc;
> > bool has_consumers = false, remove = true;
> > @@ -2582,6 +2582,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > __u64 cookie = 0;
> > int rc = 0;
> >
> > + if (is_unique)
> > + *is_unique |= uc->is_unique;
> > +
> > if (uc->handler) {
> > rc = uc->handler(uc, regs, &cookie);
> > WARN(rc < 0 || rc > 2,
> > @@ -2735,6 +2738,7 @@ static void handle_swbp(struct pt_regs *regs)
> > {
> > struct uprobe *uprobe;
> > unsigned long bp_vaddr;
> > + bool is_unique = false;
> > int is_swbp;
> >
> > bp_vaddr = uprobe_get_swbp_addr(regs);
> > @@ -2789,7 +2793,10 @@ static void handle_swbp(struct pt_regs *regs)
> > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > goto out;
> >
> > - handler_chain(uprobe, regs);
> > + handler_chain(uprobe, regs, &is_unique);
> > +
> > + if (is_unique && instruction_pointer(regs) != bp_vaddr)
> > + goto out;
> >
> > /* Try to optimize after first hit. */
> > arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
> > @@ -2819,7 +2826,7 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
> > return;
> > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > return;
> > - handler_chain(uprobe, regs);
> > + handler_chain(uprobe, regs, NULL);
> > }
> >
> > /*
> > --
> > 2.51.0
> >
>
On 09/03, Jiri Olsa wrote: > > On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote: > > On 09/02, Jiri Olsa wrote: > > > > > > If user decided to take execution elsewhere, it makes little sense > > > to execute the original instruction, so let's skip it. > > > > Exactly. > > > > So why do we need all these "is_unique" complications? Only a single > > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp() > > can just do > > > > handler_chain(uprobe, regs); > > if (instruction_pointer(regs) != bp_vaddr) > > goto out; > > hum, that's what I did in rfc [1] but I thought you did not like that [2] > > [1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/ > [2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/ > > I guess I misunderstood your reply [2], I'd be happy to drop the > unique/exclusive flag Well, but that rfc didn't introduce the exclusive consumers, and I think we agree that even with these changes the non-exclusive consumers must never change regs->ip? > > But if a non-exclusive consumer changes regs->ip, we have a problem > > anyway, right? > > > > We can probably add something like > > > > rc = uc->handler(uc, regs, &cookie); > > + WARN_ON(!uc->is_unique && instruction_pointer(regs) != bp_vaddr); > > > > into handler_chain(), although I don't think this is needed. Oleg.
On Thu, Sep 04, 2025 at 10:49:50AM +0200, Oleg Nesterov wrote: > On 09/03, Jiri Olsa wrote: > > > > On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote: > > > On 09/02, Jiri Olsa wrote: > > > > > > > > If user decided to take execution elsewhere, it makes little sense > > > > to execute the original instruction, so let's skip it. > > > > > > Exactly. > > > > > > So why do we need all these "is_unique" complications? Only a single > > > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp() > > > can just do > > > > > > handler_chain(uprobe, regs); > > > if (instruction_pointer(regs) != bp_vaddr) > > > goto out; > > > > hum, that's what I did in rfc [1] but I thought you did not like that [2] > > > > [1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/ > > [2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/ > > > > I guess I misunderstood your reply [2], I'd be happy to drop the > > unique/exclusive flag > > Well, but that rfc didn't introduce the exclusive consumers, and I think > we agree that even with these changes the non-exclusive consumers must > never change regs->ip? ok, got excited too soon.. so you meant getting rid of is_unique check only for this patch and have just change below.. but keep the unique/exclusive flag from patch#1 IIUC Andrii would remove the unique flag completely? jirka -- diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index b9b088f7333a..1baf5d2792ff 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -2791,6 +2791,9 @@ static void handle_swbp(struct pt_regs *regs) handler_chain(uprobe, regs); + if (instruction_pointer(regs) != bp_vaddr) + goto out; + /* Try to optimize after first hit. */ arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
On 09/04, Jiri Olsa wrote: > > On Thu, Sep 04, 2025 at 10:49:50AM +0200, Oleg Nesterov wrote: > > On 09/03, Jiri Olsa wrote: > > > > > > On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote: > > > > On 09/02, Jiri Olsa wrote: > > > > > > > > > > If user decided to take execution elsewhere, it makes little sense > > > > > to execute the original instruction, so let's skip it. > > > > > > > > Exactly. > > > > > > > > So why do we need all these "is_unique" complications? Only a single > > > > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp() > > > > can just do > > > > > > > > handler_chain(uprobe, regs); > > > > if (instruction_pointer(regs) != bp_vaddr) > > > > goto out; > > > > > > hum, that's what I did in rfc [1] but I thought you did not like that [2] > > > > > > [1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/ > > > [2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/ > > > > > > I guess I misunderstood your reply [2], I'd be happy to drop the > > > unique/exclusive flag > > > > Well, but that rfc didn't introduce the exclusive consumers, and I think > > we agree that even with these changes the non-exclusive consumers must > > never change regs->ip? > > ok, got excited too soon.. so you meant getting rid of is_unique > check only for this patch and have just change below.. but keep > the unique/exclusive flag from patch#1 Yes, this is what I meant, > IIUC Andrii would remove the unique flag completely? Lets wait for Andrii... Oleg.
On Thu, Sep 4, 2025 at 4:26 AM Oleg Nesterov <oleg@redhat.com> wrote: > > On 09/04, Jiri Olsa wrote: > > > > On Thu, Sep 04, 2025 at 10:49:50AM +0200, Oleg Nesterov wrote: > > > On 09/03, Jiri Olsa wrote: > > > > > > > > On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote: > > > > > On 09/02, Jiri Olsa wrote: > > > > > > > > > > > > If user decided to take execution elsewhere, it makes little sense > > > > > > to execute the original instruction, so let's skip it. > > > > > > > > > > Exactly. > > > > > > > > > > So why do we need all these "is_unique" complications? Only a single > > > > > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp() > > > > > can just do > > > > > > > > > > handler_chain(uprobe, regs); > > > > > if (instruction_pointer(regs) != bp_vaddr) > > > > > goto out; > > > > > > > > hum, that's what I did in rfc [1] but I thought you did not like that [2] > > > > > > > > [1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/ > > > > [2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/ > > > > > > > > I guess I misunderstood your reply [2], I'd be happy to drop the > > > > unique/exclusive flag > > > > > > Well, but that rfc didn't introduce the exclusive consumers, and I think > > > we agree that even with these changes the non-exclusive consumers must > > > never change regs->ip? > > > > ok, got excited too soon.. so you meant getting rid of is_unique > > check only for this patch and have just change below.. but keep > > the unique/exclusive flag from patch#1 > > Yes, this is what I meant, > > > IIUC Andrii would remove the unique flag completely? > > Lets wait for Andrii... Not Andrii, but I see only negatives in this extra flag. It doesn't add any safety or guardrails. No need to pollute uapi with pointless flags.
On Thu, Sep 4, 2025 at 8:02 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Sep 4, 2025 at 4:26 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 09/04, Jiri Olsa wrote:
> > >
> > > On Thu, Sep 04, 2025 at 10:49:50AM +0200, Oleg Nesterov wrote:
> > > > On 09/03, Jiri Olsa wrote:
> > > > >
> > > > > On Wed, Sep 03, 2025 at 01:26:48PM +0200, Oleg Nesterov wrote:
> > > > > > On 09/02, Jiri Olsa wrote:
> > > > > > >
> > > > > > > If user decided to take execution elsewhere, it makes little sense
> > > > > > > to execute the original instruction, so let's skip it.
> > > > > >
> > > > > > Exactly.
> > > > > >
> > > > > > So why do we need all these "is_unique" complications? Only a single
> > > > > > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp()
> > > > > > can just do
> > > > > >
> > > > > > handler_chain(uprobe, regs);
> > > > > > if (instruction_pointer(regs) != bp_vaddr)
> > > > > > goto out;
> > > > >
> > > > > hum, that's what I did in rfc [1] but I thought you did not like that [2]
> > > > >
> > > > > [1] https://lore.kernel.org/bpf/20250801210238.2207429-2-jolsa@kernel.org/
> > > > > [2] https://lore.kernel.org/bpf/20250802103426.GC31711@redhat.com/
> > > > >
> > > > > I guess I misunderstood your reply [2], I'd be happy to drop the
> > > > > unique/exclusive flag
> > > >
> > > > Well, but that rfc didn't introduce the exclusive consumers, and I think
> > > > we agree that even with these changes the non-exclusive consumers must
> > > > never change regs->ip?
> > >
> > > ok, got excited too soon.. so you meant getting rid of is_unique
> > > check only for this patch and have just change below.. but keep
> > > the unique/exclusive flag from patch#1
> >
> > Yes, this is what I meant,
> >
> > > IIUC Andrii would remove the unique flag completely?
> >
> > Lets wait for Andrii...
>
> Not Andrii, but I see only negatives in this extra flag.
> It doesn't add any safety or guardrails.
> No need to pollute uapi with pointless flags.
+1. I think it's fine to just have something like
if (unlikely(instruction_pointer(regs) != bp_vaddr))
goto out;
after all uprobe callbacks were processed. Even if every single one of
them modify IP, the last one that did that wins. Others (if they care)
can detect this.
Generally speaking, this is a very specialized use case (which is why
the opposition to complicating UAPI for all of that), and I'd expect
to have at most 1 such uprobe callbacks at any attach point, while all
others (if there are any "others") are read-only and won't care about
return IP.
On 09/04, Andrii Nakryiko wrote: > > On Thu, Sep 4, 2025 at 8:02 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Sep 4, 2025 at 4:26 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > On 09/04, Jiri Olsa wrote: > > > > > > > > > > > > ok, got excited too soon.. so you meant getting rid of is_unique > > > > check only for this patch and have just change below.. but keep > > > > the unique/exclusive flag from patch#1 > > > > > > Yes, this is what I meant, > > > > > > > IIUC Andrii would remove the unique flag completely? > > > > > > Lets wait for Andrii... > > > > Not Andrii, but I see only negatives in this extra flag. > > It doesn't add any safety or guardrails. > > No need to pollute uapi with pointless flags. > > +1. I think it's fine to just have something like > > if (unlikely(instruction_pointer(regs) != bp_vaddr)) > goto out; > > after all uprobe callbacks were processed. Even if every single one of > them modify IP, the last one that did that wins. OK. If any consumer can change regs->ip, then I can only repeat: Yes... but what if we there are multiple consumers? The 1st one changes instruction_pointer, the next is unaware. Or it may change regs->ip too... > Others (if they care) > can detect this. How? If the the consumer which changes regs->ip is not the 1st one? That said. If you guys don't see a problem - I won't even try to argue. As I said many times, I have no idea how people actually use uprobes ;) Oleg.
On Thu, Sep 4, 2025 at 11:57 AM Oleg Nesterov <oleg@redhat.com> wrote: > > On 09/04, Andrii Nakryiko wrote: > > > > On Thu, Sep 4, 2025 at 8:02 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Sep 4, 2025 at 4:26 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > On 09/04, Jiri Olsa wrote: > > > > > > > > > > > > > > > ok, got excited too soon.. so you meant getting rid of is_unique > > > > > check only for this patch and have just change below.. but keep > > > > > the unique/exclusive flag from patch#1 > > > > > > > > Yes, this is what I meant, > > > > > > > > > IIUC Andrii would remove the unique flag completely? > > > > > > > > Lets wait for Andrii... > > > > > > Not Andrii, but I see only negatives in this extra flag. > > > It doesn't add any safety or guardrails. > > > No need to pollute uapi with pointless flags. > > > > +1. I think it's fine to just have something like > > > > if (unlikely(instruction_pointer(regs) != bp_vaddr)) > > goto out; > > > > after all uprobe callbacks were processed. Even if every single one of > > them modify IP, the last one that did that wins. > > OK. If any consumer can change regs->ip, then I can only repeat: > > Yes... but what if we there are multiple consumers? The 1st one changes > instruction_pointer, the next is unaware. Or it may change regs->ip too... > > > Others (if they care) > > can detect this. > > How? If the the consumer which changes regs->ip is not the 1st one? > We are probably speaking past each other. uprobe consumers (including BPF ones) see struct pt_regs, so they get what's the latest regs->ip. Sure, they won't know that it was changed, but oh well, not sure that matters all that much. And if it does matter, then we can solve that by giving users ability to carefully order consumers (we have similar problems and some solutions for that in BPF for some other BPF programs; it just never been necessary for uprobes/kprobes specifically). > That said. If you guys don't see a problem - I won't even try to argue. I don't, yep. > > As I said many times, I have no idea how people actually use uprobes ;) > > Oleg. >
On 09/04, Andrii Nakryiko wrote: > > On Thu, Sep 4, 2025 at 11:57 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > That said. If you guys don't see a problem - I won't even try to argue. > > I don't, yep. OK. Then I am fine with that too. Oleg.
On Wed, Sep 3, 2025 at 4:28 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 09/02, Jiri Olsa wrote:
> >
> > If user decided to take execution elsewhere, it makes little sense
> > to execute the original instruction, so let's skip it.
>
> Exactly.
>
> So why do we need all these "is_unique" complications? Only a single
I second this. This whole is_unique flag just seems like an
unnecessary thing that spills all around (extra kernel and libbpf
flags/APIs), and it's all just not to confuse the second uprobe
attached? Let's just allow uprobes to override user registers and
handle IP change on kernel side (as unlikely() check)?
> is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp()
> can just do
>
> handler_chain(uprobe, regs);
> if (instruction_pointer(regs) != bp_vaddr)
> goto out;
>
>
> > Allowing this
> > behaviour only for uprobe with unique consumer attached.
>
> But if a non-exclusive consumer changes regs->ip, we have a problem
> anyway, right?
>
> We can probably add something like
>
> rc = uc->handler(uc, regs, &cookie);
> + WARN_ON(!uc->is_unique && instruction_pointer(regs) != bp_vaddr);
>
> into handler_chain(), although I don't think this is needed.
>
> Oleg.
>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/events/uprobes.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index b9b088f7333a..da8291941c6b 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2568,7 +2568,7 @@ static bool ignore_ret_handler(int rc)
> > return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
> > }
> >
> > -static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > +static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
> > {
> > struct uprobe_consumer *uc;
> > bool has_consumers = false, remove = true;
> > @@ -2582,6 +2582,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > __u64 cookie = 0;
> > int rc = 0;
> >
> > + if (is_unique)
> > + *is_unique |= uc->is_unique;
> > +
> > if (uc->handler) {
> > rc = uc->handler(uc, regs, &cookie);
> > WARN(rc < 0 || rc > 2,
> > @@ -2735,6 +2738,7 @@ static void handle_swbp(struct pt_regs *regs)
> > {
> > struct uprobe *uprobe;
> > unsigned long bp_vaddr;
> > + bool is_unique = false;
> > int is_swbp;
> >
> > bp_vaddr = uprobe_get_swbp_addr(regs);
> > @@ -2789,7 +2793,10 @@ static void handle_swbp(struct pt_regs *regs)
> > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > goto out;
> >
> > - handler_chain(uprobe, regs);
> > + handler_chain(uprobe, regs, &is_unique);
> > +
> > + if (is_unique && instruction_pointer(regs) != bp_vaddr)
> > + goto out;
> >
> > /* Try to optimize after first hit. */
> > arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
> > @@ -2819,7 +2826,7 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
> > return;
> > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > return;
> > - handler_chain(uprobe, regs);
> > + handler_chain(uprobe, regs, NULL);
> > }
> >
> > /*
> > --
> > 2.51.0
> >
>
On Wed, Sep 03, 2025 at 11:20:01AM -0700, Andrii Nakryiko wrote:
> On Wed, Sep 3, 2025 at 4:28 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 09/02, Jiri Olsa wrote:
> > >
> > > If user decided to take execution elsewhere, it makes little sense
> > > to execute the original instruction, so let's skip it.
> >
> > Exactly.
> >
> > So why do we need all these "is_unique" complications? Only a single
>
> I second this. This whole is_unique flag just seems like an
> unnecessary thing that spills all around (extra kernel and libbpf
> flags/APIs), and it's all just not to confuse the second uprobe
> attached? Let's just allow uprobes to override user registers and
> handle IP change on kernel side (as unlikely() check)?
yes! ;-) I'd just refresh rfc version then
thanks,
jirka
>
> > is_unique/exclusive consumer can change regs->ip, so I guess handle_swbp()
> > can just do
> >
> > handler_chain(uprobe, regs);
> > if (instruction_pointer(regs) != bp_vaddr)
> > goto out;
> >
> >
> > > Allowing this
> > > behaviour only for uprobe with unique consumer attached.
> >
> > But if a non-exclusive consumer changes regs->ip, we have a problem
> > anyway, right?
> >
> > We can probably add something like
> >
> > rc = uc->handler(uc, regs, &cookie);
> > + WARN_ON(!uc->is_unique && instruction_pointer(regs) != bp_vaddr);
> >
> > into handler_chain(), although I don't think this is needed.
> >
> > Oleg.
> >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > kernel/events/uprobes.c | 13 ++++++++++---
> > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index b9b088f7333a..da8291941c6b 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -2568,7 +2568,7 @@ static bool ignore_ret_handler(int rc)
> > > return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
> > > }
> > >
> > > -static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > > +static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
> > > {
> > > struct uprobe_consumer *uc;
> > > bool has_consumers = false, remove = true;
> > > @@ -2582,6 +2582,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> > > __u64 cookie = 0;
> > > int rc = 0;
> > >
> > > + if (is_unique)
> > > + *is_unique |= uc->is_unique;
> > > +
> > > if (uc->handler) {
> > > rc = uc->handler(uc, regs, &cookie);
> > > WARN(rc < 0 || rc > 2,
> > > @@ -2735,6 +2738,7 @@ static void handle_swbp(struct pt_regs *regs)
> > > {
> > > struct uprobe *uprobe;
> > > unsigned long bp_vaddr;
> > > + bool is_unique = false;
> > > int is_swbp;
> > >
> > > bp_vaddr = uprobe_get_swbp_addr(regs);
> > > @@ -2789,7 +2793,10 @@ static void handle_swbp(struct pt_regs *regs)
> > > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > > goto out;
> > >
> > > - handler_chain(uprobe, regs);
> > > + handler_chain(uprobe, regs, &is_unique);
> > > +
> > > + if (is_unique && instruction_pointer(regs) != bp_vaddr)
> > > + goto out;
> > >
> > > /* Try to optimize after first hit. */
> > > arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
> > > @@ -2819,7 +2826,7 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
> > > return;
> > > if (arch_uprobe_ignore(&uprobe->arch, regs))
> > > return;
> > > - handler_chain(uprobe, regs);
> > > + handler_chain(uprobe, regs, NULL);
> > > }
> > >
> > > /*
> > > --
> > > 2.51.0
> > >
> >
© 2016 - 2025 Red Hat, Inc.