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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
> > >
> >
