All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <olsajiri@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
	Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, x86@kernel.org,
	Yonghong Song <yhs@fb.com>, Song Liu <songliubraving@fb.com>,
	Andrii Nakryiko <andrii@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mahe Tardy <mahe.tardy@gmail.com>
Subject: Re: [BUG/RFC 1/2] arm64/ftrace,bpf: Fix partial regs after bpf_prog_run
Date: Wed, 7 Jan 2026 09:23:01 +0100	[thread overview]
Message-ID: <aV4X5Yx07LomQji4@krava> (raw)
In-Reply-To: <20260105162220.6ba5129a@gandalf.local.home>

On Mon, Jan 05, 2026 at 04:22:20PM -0500, Steven Rostedt wrote:
> On Sun, 4 Jan 2026 22:34:15 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > This looks a bit grotty to me and presumably other architectures would
> > > need similar treatement. Wouldn't it be cleaner to reuse the existing
> > > API instead? For example, by calling ftrace_regs_set_instruction_pointer()
> > > and ftrace_regs_set_return_value() to update the relevant registers from
> > > the core code?  
> > 
> > I agreed with using the generic APIs. Also, ftrace_partial_regs_fix() is
> > not self-explained. Maybe ftrace_regs_set_by_regs()?
> 
> Or perhaps: ftrace_partial_regs_update() where you call it if you need to
> update the regs.
> 
> /*
>  * ftrace_partial_regs_update - update the original ftrace_regs from regs
>  * @fregs: The ftrace_regs to update from @regs
>  * @regs: The partial regs from ftrace_partial_regs() that was updated
>  *
>  * Some architectures have the partial regs living in the ftrace_regs
>  * structure, whereas other architectures need to make a different copy
>  * of the @regs. If a partial @regs is retrieved by ftrace_partial_regs() and
>  * if the code using @regs updates a field (like the instruction pointer or
>  * stack pointer) it may need to propagate that change to the original @fregs
>  * it retrieved the partial @regs from. Use this function to guarantee that
>  * update happens.
>  */
> static __always_inline void
> ftrace_partial_regs_update(const struct ftrace_regs *fregs, struct pt_regs *regs) {
> 	struct __arch_ftrace_regs *afregs = arch_ftrace_regs(fregs);
> 
> 	if (afregs->pc != regs->pc) {
> 		afregs->pc = regs->pc;
> 		afregs->regs[0] = regs->regs[0];
> 	}
> }
> 
> -- Steve

lgtm, the full change is below, I'll send new version of the patchset

thanks,
jirka


---
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 1621c84f44b3..177c7bbf3b84 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -157,6 +157,30 @@ ftrace_partial_regs(const struct ftrace_regs *fregs, struct pt_regs *regs)
 	return regs;
 }
 
+/*
+ * ftrace_partial_regs_update - update the original ftrace_regs from regs
+ * @fregs: The ftrace_regs to update from @regs
+ * @regs: The partial regs from ftrace_partial_regs() that was updated
+ *
+ * Some architectures have the partial regs living in the ftrace_regs
+ * structure, whereas other architectures need to make a different copy
+ * of the @regs. If a partial @regs is retrieved by ftrace_partial_regs() and
+ * if the code using @regs updates a field (like the instruction pointer or
+ * stack pointer) it may need to propagate that change to the original @fregs
+ * it retrieved the partial @regs from. Use this function to guarantee that
+ * update happens.
+ */
+static __always_inline void
+ftrace_partial_regs_update(const struct ftrace_regs *fregs, struct pt_regs *regs)
+{
+	struct __arch_ftrace_regs *afregs = arch_ftrace_regs(fregs);
+
+	if (afregs->pc != regs->pc) {
+		afregs->pc = regs->pc;
+		afregs->regs[0] = regs->regs[0];
+	}
+}
+
 #define arch_ftrace_fill_perf_regs(fregs, _regs) do {		\
 		(_regs)->pc = arch_ftrace_regs(fregs)->pc;			\
 		(_regs)->regs[29] = arch_ftrace_regs(fregs)->fp;		\
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 770f0dc993cc..ae22559b4099 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -213,6 +213,9 @@ ftrace_partial_regs(struct ftrace_regs *fregs, struct pt_regs *regs)
 	return regs;
 }
 
+static __always_inline void
+ftrace_partial_regs_update(struct ftrace_regs *fregs, struct pt_regs *regs) { }
+
 #endif /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS || CONFIG_HAVE_FTRACE_REGS_HAVING_PT_REGS */
 
 #ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 6e076485bf70..3a17f79b20c2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2564,6 +2564,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 	old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx);
 	err = bpf_prog_run(link->link.prog, regs);
 	bpf_reset_run_ctx(old_run_ctx);
+	ftrace_partial_regs_update(fregs, bpf_kprobe_multi_pt_regs_ptr());
 	rcu_read_unlock();
 
  out:
-- 
2.52.0


  reply	other threads:[~2026-01-07  8:23 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-05 12:59 [BUG/RFC 1/2] arm64/ftrace,bpf: Fix partial regs after bpf_prog_run Jiri Olsa
2025-11-05 12:59 ` [PATCH 2/2] selftests/bpf: Add test for bpf_override_return helper Jiri Olsa
2025-11-05 22:04   ` Song Liu
2025-11-13  7:51 ` [BUG/RFC 1/2] arm64/ftrace,bpf: Fix partial regs after bpf_prog_run Jiri Olsa
2026-01-02 14:52 ` Will Deacon
2026-01-04 11:56   ` Jiri Olsa
2026-01-04 13:34   ` Masami Hiramatsu
2026-01-05 21:22     ` Steven Rostedt
2026-01-07  8:23       ` Jiri Olsa [this message]
2026-01-07 15:53         ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aV4X5Yx07LomQji4@krava \
    --to=olsajiri@gmail.com \
    --cc=andrii@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=mahe.tardy@gmail.com \
    --cc=mark.rutland@arm.com \
    --cc=mhiramat@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=songliubraving@fb.com \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.