| Message ID | 20250318204841.373116-7-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Enable UPROBES with GCS | expand |
On Tue, Mar 18, 2025 at 03:48:40PM -0500, Jeremy Linton wrote: > The uprobe_warn function is limited to the uprobe core, > but the functionality is useful to report arch specific errors. > > Drop the static so it can be used in those code paths. ...and also make use of it in the arm64 code.
On Tue, Mar 18, 2025 at 03:48:40PM -0500, Jeremy Linton wrote: > The uprobe_warn function is limited to the uprobe core, > but the functionality is useful to report arch specific errors. > > Drop the static so it can be used in those code paths. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> NAK to the arm64 additions. There is no reason to print a warning message for something that does not adversely affect the kernel, and where buggy or malicious userspace can trigger the warning when the kernel is behaving correctly. This isn't even ratelimited. Mark. > --- > arch/arm64/kernel/probes/simulate-insn.c | 8 ++++++-- > arch/arm64/kernel/probes/uprobes.c | 4 ++++ > include/linux/uprobes.h | 1 + > kernel/events/uprobes.c | 2 +- > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c > index 1fc9bb69b1eb..fe637fec8f36 100644 > --- a/arch/arm64/kernel/probes/simulate-insn.c > +++ b/arch/arm64/kernel/probes/simulate-insn.c > @@ -56,8 +56,10 @@ static inline void update_lr(struct pt_regs *regs, long addr) > > if (user_mode(regs) && task_gcs_el0_enabled(current)) { > push_user_gcs(addr + 4, &err); > - if (err) > + if (err) { > + uprobe_warn(current, "GCS stack push failure"); > force_sig(SIGSEGV); > + } > } > procedure_link_pointer_set(regs, addr + 4); > } > @@ -160,8 +162,10 @@ simulate_ret(u32 opcode, long addr, struct pt_regs *regs) > > if (user_mode(regs) && task_gcs_el0_enabled(current)) { > ret_addr = pop_user_gcs(&err); > - if (err || ret_addr != procedure_link_pointer(regs)) > + if (err || ret_addr != procedure_link_pointer(regs)) { > + uprobe_warn(current, "GCS RET address mismatch"); > force_sig(SIGSEGV); > + } > } > > } > diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c > index 5e72409a255a..9349d521316c 100644 > --- a/arch/arm64/kernel/probes/uprobes.c > +++ b/arch/arm64/kernel/probes/uprobes.c > @@ -54,6 +54,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > > switch (arm_probe_decode_insn(insn, &auprobe->api)) { > case INSN_REJECTED: > + uprobe_warn(current, "Unsupported instruction at probe location"); > return -EINVAL; > > case INSN_GOOD_NO_SLOT: > @@ -169,6 +170,7 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, > gcspr = read_sysreg_s(SYS_GCSPR_EL0); > gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err); > if (err) { > + uprobe_warn(current, "GCS stack not available for retprobe"); > force_sig(SIGSEGV); > goto out; > } > @@ -180,11 +182,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, > * a GCS exception. > */ > if (gcs_ret_vaddr != orig_ret_vaddr) { > + uprobe_warn(current, "LR/GCS mismatch, likely due to incorrectly placed retprobe"); > orig_ret_vaddr = -1; > goto out; > } > put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err); > if (err) { > + uprobe_warn(current, "GCS stack update failure during retprobe"); > force_sig(SIGSEGV); > goto out; > } > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index b1df7d792fa1..9578ef1ea5a3 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -185,6 +185,7 @@ struct uprobes_state { > }; > > extern void __init uprobes_init(void); > +extern void uprobe_warn(struct task_struct *t, const char *msg); > extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); > extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); > extern bool is_swbp_insn(uprobe_opcode_t *insn); > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index b4ca8898fe17..613c1c76f227 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -118,7 +118,7 @@ struct xol_area { > unsigned long vaddr; /* Page(s) of instruction slots */ > }; > > -static void uprobe_warn(struct task_struct *t, const char *msg) > +void uprobe_warn(struct task_struct *t, const char *msg) > { > pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg); > } > -- > 2.48.1 >
On 03/18, Jeremy Linton wrote: > > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -185,6 +185,7 @@ struct uprobes_state { > }; > > extern void __init uprobes_init(void); > +extern void uprobe_warn(struct task_struct *t, const char *msg); > extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); > extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); > extern bool is_swbp_insn(uprobe_opcode_t *insn); > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index b4ca8898fe17..613c1c76f227 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -118,7 +118,7 @@ struct xol_area { > unsigned long vaddr; /* Page(s) of instruction slots */ > }; > > -static void uprobe_warn(struct task_struct *t, const char *msg) > +void uprobe_warn(struct task_struct *t, const char *msg) > { > pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg); > } Oh, no, please don't. uprobe_warn() is ugly and needs changes. If nothing else it doesn't even use its "t" argument. Oleg.
Hi, On 3/19/25 9:51 AM, Oleg Nesterov wrote: > On 03/18, Jeremy Linton wrote: >> >> --- a/include/linux/uprobes.h >> +++ b/include/linux/uprobes.h >> @@ -185,6 +185,7 @@ struct uprobes_state { >> }; >> >> extern void __init uprobes_init(void); >> +extern void uprobe_warn(struct task_struct *t, const char *msg); >> extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); >> extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); >> extern bool is_swbp_insn(uprobe_opcode_t *insn); >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c >> index b4ca8898fe17..613c1c76f227 100644 >> --- a/kernel/events/uprobes.c >> +++ b/kernel/events/uprobes.c >> @@ -118,7 +118,7 @@ struct xol_area { >> unsigned long vaddr; /* Page(s) of instruction slots */ >> }; >> >> -static void uprobe_warn(struct task_struct *t, const char *msg) >> +void uprobe_warn(struct task_struct *t, const char *msg) >> { >> pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg); >> } > > Oh, no, please don't. > > uprobe_warn() is ugly and needs changes. If nothing else it doesn't even use > its "t" argument. Ha, I didn't look that closely at it. That is basically the same bug 1/7 here is fixing for the gcs task function! This is in its own patch to allow it to be easily dropped, which is what will happen as I'm aware of previous variations of this discussion. While Mark R's perspective is valid, it remains worthwhile to again point out that the uprobes subsystem (and a few like it) is a bit of a mystery box. Some of these error conditions are very opaque for a user who isn't also sufficiently familiar with uprobes to be able to both find the code as well as understand or instrument it when it tosses an error. Discoverability is made more difficult by the extensive use of inline/static. End users try to work around this. For example Brendan Gregg's perf-tools uprobe wrapper will dump the last two lines of the kernel log when it hits unexpected errors.
diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c index 1fc9bb69b1eb..fe637fec8f36 100644 --- a/arch/arm64/kernel/probes/simulate-insn.c +++ b/arch/arm64/kernel/probes/simulate-insn.c @@ -56,8 +56,10 @@ static inline void update_lr(struct pt_regs *regs, long addr) if (user_mode(regs) && task_gcs_el0_enabled(current)) { push_user_gcs(addr + 4, &err); - if (err) + if (err) { + uprobe_warn(current, "GCS stack push failure"); force_sig(SIGSEGV); + } } procedure_link_pointer_set(regs, addr + 4); } @@ -160,8 +162,10 @@ simulate_ret(u32 opcode, long addr, struct pt_regs *regs) if (user_mode(regs) && task_gcs_el0_enabled(current)) { ret_addr = pop_user_gcs(&err); - if (err || ret_addr != procedure_link_pointer(regs)) + if (err || ret_addr != procedure_link_pointer(regs)) { + uprobe_warn(current, "GCS RET address mismatch"); force_sig(SIGSEGV); + } } } diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c index 5e72409a255a..9349d521316c 100644 --- a/arch/arm64/kernel/probes/uprobes.c +++ b/arch/arm64/kernel/probes/uprobes.c @@ -54,6 +54,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, switch (arm_probe_decode_insn(insn, &auprobe->api)) { case INSN_REJECTED: + uprobe_warn(current, "Unsupported instruction at probe location"); return -EINVAL; case INSN_GOOD_NO_SLOT: @@ -169,6 +170,7 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, gcspr = read_sysreg_s(SYS_GCSPR_EL0); gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err); if (err) { + uprobe_warn(current, "GCS stack not available for retprobe"); force_sig(SIGSEGV); goto out; } @@ -180,11 +182,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, * a GCS exception. */ if (gcs_ret_vaddr != orig_ret_vaddr) { + uprobe_warn(current, "LR/GCS mismatch, likely due to incorrectly placed retprobe"); orig_ret_vaddr = -1; goto out; } put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err); if (err) { + uprobe_warn(current, "GCS stack update failure during retprobe"); force_sig(SIGSEGV); goto out; } diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index b1df7d792fa1..9578ef1ea5a3 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -185,6 +185,7 @@ struct uprobes_state { }; extern void __init uprobes_init(void); +extern void uprobe_warn(struct task_struct *t, const char *msg); extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); extern bool is_swbp_insn(uprobe_opcode_t *insn); diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index b4ca8898fe17..613c1c76f227 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -118,7 +118,7 @@ struct xol_area { unsigned long vaddr; /* Page(s) of instruction slots */ }; -static void uprobe_warn(struct task_struct *t, const char *msg) +void uprobe_warn(struct task_struct *t, const char *msg) { pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg); }
The uprobe_warn function is limited to the uprobe core, but the functionality is useful to report arch specific errors. Drop the static so it can be used in those code paths. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/kernel/probes/simulate-insn.c | 8 ++++++-- arch/arm64/kernel/probes/uprobes.c | 4 ++++ include/linux/uprobes.h | 1 + kernel/events/uprobes.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-)