[PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed

Jiri Olsa posted 6 patches 2 weeks, 2 days ago
[PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Jiri Olsa 2 weeks, 2 days ago
If uprobe handler changes instruction pointer we still execute single
step) or emulate the original instruction and increment the (new) ip
with its length.

This makes the new instruction pointer bogus and application will
likely crash on illegal instruction execution.

If user decided to take execution elsewhere, it makes little sense
to execute the original instruction, so let's skip it.

Acked-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/uprobes.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7ca1940607bd..2b32c32bcb77 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
 
 	handler_chain(uprobe, regs);
 
+	/*
+	 * If user decided to take execution elsewhere, it makes little sense
+	 * to execute the original instruction, so let's skip it.
+	 */
+	if (instruction_pointer(regs) != bp_vaddr)
+		goto out;
+
 	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;
 
-- 
2.51.0
Re: [PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Andrii Nakryiko 2 weeks, 2 days ago
On Tue, Sep 16, 2025 at 2:53 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> If uprobe handler changes instruction pointer we still execute single
> step) or emulate the original instruction and increment the (new) ip
> with its length.
>
> This makes the new instruction pointer bogus and application will
> likely crash on illegal instruction execution.
>
> If user decided to take execution elsewhere, it makes little sense
> to execute the original instruction, so let's skip it.
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/uprobes.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7ca1940607bd..2b32c32bcb77 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
>
>         handler_chain(uprobe, regs);
>
> +       /*
> +        * If user decided to take execution elsewhere, it makes little sense
> +        * to execute the original instruction, so let's skip it.
> +        */
> +       if (instruction_pointer(regs) != bp_vaddr)
> +               goto out;
> +

Peter, Ingo,

Are you guys ok with us routing this through the bpf-next tree? We'll
have a tiny conflict because in perf/core branch there is
arch_uprobe_optimize() call added after handler_chain(), so git merge
will be a bit confused, probably. But it should be trivially
resolvable.

>         if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
>                 goto out;
>
> --
> 2.51.0
>
Re: [PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Peter Zijlstra 1 week, 1 day ago
On Tue, Sep 16, 2025 at 03:28:52PM -0700, Andrii Nakryiko wrote:
> On Tue, Sep 16, 2025 at 2:53 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > If uprobe handler changes instruction pointer we still execute single
> > step) or emulate the original instruction and increment the (new) ip
> > with its length.
> >
> > This makes the new instruction pointer bogus and application will
> > likely crash on illegal instruction execution.
> >
> > If user decided to take execution elsewhere, it makes little sense
> > to execute the original instruction, so let's skip it.
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/events/uprobes.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 7ca1940607bd..2b32c32bcb77 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
> >
> >         handler_chain(uprobe, regs);
> >
> > +       /*
> > +        * If user decided to take execution elsewhere, it makes little sense
> > +        * to execute the original instruction, so let's skip it.
> > +        */
> > +       if (instruction_pointer(regs) != bp_vaddr)
> > +               goto out;
> > +
> 
> Peter, Ingo,
> 
> Are you guys ok with us routing this through the bpf-next tree? We'll
> have a tiny conflict because in perf/core branch there is
> arch_uprobe_optimize() call added after handler_chain(), so git merge
> will be a bit confused, probably. But it should be trivially
> resolvable.

Nah, I suppose that'll be fine. Thanks!
Re: [PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Alexei Starovoitov 1 week, 1 day ago
On Wed, Sep 24, 2025 at 11:15 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Sep 16, 2025 at 03:28:52PM -0700, Andrii Nakryiko wrote:
> > On Tue, Sep 16, 2025 at 2:53 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > If uprobe handler changes instruction pointer we still execute single
> > > step) or emulate the original instruction and increment the (new) ip
> > > with its length.
> > >
> > > This makes the new instruction pointer bogus and application will
> > > likely crash on illegal instruction execution.
> > >
> > > If user decided to take execution elsewhere, it makes little sense
> > > to execute the original instruction, so let's skip it.
> > >
> > > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  kernel/events/uprobes.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index 7ca1940607bd..2b32c32bcb77 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
> > >
> > >         handler_chain(uprobe, regs);
> > >
> > > +       /*
> > > +        * If user decided to take execution elsewhere, it makes little sense
> > > +        * to execute the original instruction, so let's skip it.
> > > +        */
> > > +       if (instruction_pointer(regs) != bp_vaddr)
> > > +               goto out;
> > > +
> >
> > Peter, Ingo,
> >
> > Are you guys ok with us routing this through the bpf-next tree? We'll
> > have a tiny conflict because in perf/core branch there is
> > arch_uprobe_optimize() call added after handler_chain(), so git merge
> > will be a bit confused, probably. But it should be trivially
> > resolvable.
>
> Nah, I suppose that'll be fine. Thanks!

Thanks! Applied.

Jiri,
in the future, please keep the whole history in the cover letter.
v1->v2, v2->v3. Just v4 changes are nice, but pls copy paste
previous cover letters and expand them.
Also please always include links to previous versions in the cover.
Search on lore sucks. Links in the cover are a much better
way to preserve the history.
Re: [PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Jiri Olsa 1 week, 1 day ago
On Wed, Sep 24, 2025 at 11:47:42AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 24, 2025 at 11:15 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Sep 16, 2025 at 03:28:52PM -0700, Andrii Nakryiko wrote:
> > > On Tue, Sep 16, 2025 at 2:53 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > If uprobe handler changes instruction pointer we still execute single
> > > > step) or emulate the original instruction and increment the (new) ip
> > > > with its length.
> > > >
> > > > This makes the new instruction pointer bogus and application will
> > > > likely crash on illegal instruction execution.
> > > >
> > > > If user decided to take execution elsewhere, it makes little sense
> > > > to execute the original instruction, so let's skip it.
> > > >
> > > > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  kernel/events/uprobes.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > > index 7ca1940607bd..2b32c32bcb77 100644
> > > > --- a/kernel/events/uprobes.c
> > > > +++ b/kernel/events/uprobes.c
> > > > @@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
> > > >
> > > >         handler_chain(uprobe, regs);
> > > >
> > > > +       /*
> > > > +        * If user decided to take execution elsewhere, it makes little sense
> > > > +        * to execute the original instruction, so let's skip it.
> > > > +        */
> > > > +       if (instruction_pointer(regs) != bp_vaddr)
> > > > +               goto out;
> > > > +
> > >
> > > Peter, Ingo,
> > >
> > > Are you guys ok with us routing this through the bpf-next tree? We'll
> > > have a tiny conflict because in perf/core branch there is
> > > arch_uprobe_optimize() call added after handler_chain(), so git merge
> > > will be a bit confused, probably. But it should be trivially
> > > resolvable.
> >
> > Nah, I suppose that'll be fine. Thanks!
> 
> Thanks! Applied.
> 
> Jiri,
> in the future, please keep the whole history in the cover letter.
> v1->v2, v2->v3. Just v4 changes are nice, but pls copy paste
> previous cover letters and expand them.

ok

> Also please always include links to previous versions in the cover.
> Search on lore sucks. Links in the cover are a much better
> way to preserve the history.

will add them in future, thanks

jirka
Re: [PATCHv4 bpf-next 2/6] uprobe: Do not emulate/sstep original instruction when ip is changed
Posted by Andrii Nakryiko 1 week, 3 days ago
On Tue, Sep 16, 2025 at 3:28 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 16, 2025 at 2:53 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > If uprobe handler changes instruction pointer we still execute single
> > step) or emulate the original instruction and increment the (new) ip
> > with its length.
> >
> > This makes the new instruction pointer bogus and application will
> > likely crash on illegal instruction execution.
> >
> > If user decided to take execution elsewhere, it makes little sense
> > to execute the original instruction, so let's skip it.
> >
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/events/uprobes.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 7ca1940607bd..2b32c32bcb77 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2741,6 +2741,13 @@ static void handle_swbp(struct pt_regs *regs)
> >
> >         handler_chain(uprobe, regs);
> >
> > +       /*
> > +        * If user decided to take execution elsewhere, it makes little sense
> > +        * to execute the original instruction, so let's skip it.
> > +        */
> > +       if (instruction_pointer(regs) != bp_vaddr)
> > +               goto out;
> > +
>
> Peter, Ingo,
>
> Are you guys ok with us routing this through the bpf-next tree? We'll
> have a tiny conflict because in perf/core branch there is
> arch_uprobe_optimize() call added after handler_chain(), so git merge
> will be a bit confused, probably. But it should be trivially
> resolvable.

Ping. Any objections for landing this patch in bpf-next?

>
> >         if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> >                 goto out;
> >
> > --
> > 2.51.0
> >