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-jo...@kernel.org/ [2] https://lore.kernel.org/bpf/20250802103426.gc31...@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 <jo...@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 > > >
