[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250903112648.GC18799@redhat.com>
Date: Wed, 3 Sep 2025 13:26:48 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Jiri Olsa <jolsa@...nel.org>
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 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@...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
>
Powered by blists - more mailing lists