linux-kernel - Re: [PATCH perf/core 02/11] uprobes: Skip emulate/sstep on unique uprobe when ip is changed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLicCjuqchpm1h5I@krava>
Date: Wed, 3 Sep 2025 21:50:34 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	x86@...nel.org, Song Liu <songliubraving@...com>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	Hao Luo <haoluo@...gle.com>, Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH perf/core 02/11] uprobes: Skip emulate/sstep on unique
 uprobe when ip is changed

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@...nel.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
> > 
>