* [PATCH 0/6] uprobes/x86: Cleanups and fixes
@ 2025-08-21 12:28 Peter Zijlstra
2025-08-21 12:28 ` [PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args Peter Zijlstra
` (7 more replies)
0 siblings, 8 replies; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
Hi,
These are cleanups and fixes that I applied on top of Jiri's patches:
https://lkml.kernel.org/r/20250720112133.244369-1-jolsa@kernel.org
The combined lot sits in:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
Jiri was going to send me some selftest updates that might mean rebasing that
tree, but we'll see. If this all works we'll land it in -tip.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-21 12:28 ` [PATCH 2/6] uprobes/x86: Optimize is_optimize() Peter Zijlstra
` (6 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
Like uprobe_syscall_args; keep things consistent.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -311,6 +311,12 @@ static int uprobe_init_insn(struct arch_
#ifdef CONFIG_X86_64
+struct uretprobe_syscall_args {
+ unsigned long r11;
+ unsigned long cx;
+ unsigned long ax;
+};
+
asm (
".pushsection .rodata\n"
".global uretprobe_trampoline_entry\n"
@@ -324,8 +330,8 @@ asm (
"uretprobe_syscall_check:\n"
"popq %r11\n"
"popq %rcx\n"
-
- /* The uretprobe syscall replaces stored %rax value with final
+ /*
+ * The uretprobe syscall replaces stored %rax value with final
* return address, so we don't restore %rax in here and just
* call ret.
*/
@@ -366,7 +372,8 @@ static unsigned long trampoline_check_ip
SYSCALL_DEFINE0(uretprobe)
{
struct pt_regs *regs = task_pt_regs(current);
- unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+ struct uretprobe_syscall_args args;
+ unsigned long err, ip, sp, tramp;
/* If there's no trampoline, we are called from wrong place. */
tramp = uprobe_get_trampoline_vaddr();
@@ -377,15 +384,15 @@ SYSCALL_DEFINE0(uretprobe)
if (unlikely(regs->ip != trampoline_check_ip(tramp)))
goto sigill;
- err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
+ err = copy_from_user(&args, (void __user *)regs->sp, sizeof(args));
if (err)
goto sigill;
/* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */
- regs->r11 = r11_cx_ax[0];
- regs->cx = r11_cx_ax[1];
- regs->ax = r11_cx_ax[2];
- regs->sp += sizeof(r11_cx_ax);
+ regs->r11 = args.r11;
+ regs->cx = args.cx;
+ regs->ax = args.ax;
+ regs->sp += sizeof(args);
regs->orig_ax = -1;
ip = regs->ip;
@@ -401,21 +408,21 @@ SYSCALL_DEFINE0(uretprobe)
*/
if (regs->sp != sp || shstk_is_enabled())
return regs->ax;
- regs->sp -= sizeof(r11_cx_ax);
+ regs->sp -= sizeof(args);
/* for the case uprobe_consumer has changed r11/cx */
- r11_cx_ax[0] = regs->r11;
- r11_cx_ax[1] = regs->cx;
+ args.r11 = regs->r11;
+ args.cx = regs->cx;
/*
* ax register is passed through as return value, so we can use
* its space on stack for ip value and jump to it through the
* trampoline's ret instruction
*/
- r11_cx_ax[2] = regs->ip;
+ args.ax = regs->ip;
regs->ip = ip;
- err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
+ err = copy_to_user((void __user *)regs->sp, &args, sizeof(args));
if (err)
goto sigill;
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
2025-08-21 12:28 ` [PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-26 5:51 ` David Laight
2025-08-21 12:28 ` [PATCH 3/6] uprobes/x86: Accept more NOP forms Peter Zijlstra
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
Make is_optimized() return a tri-state and avoid return through
argument. This simplifies things a little.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
}
-static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
+static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
{
uprobe_opcode_t insn[5];
int err;
@@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
err = copy_from_vaddr(mm, vaddr, &insn, 5);
if (err)
return err;
- *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
- return 0;
+ return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
}
static bool should_optimize(struct arch_uprobe *auprobe)
@@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
unsigned long vaddr)
{
if (should_optimize(auprobe)) {
- bool optimized = false;
- int err;
-
/*
* We could race with another thread that already optimized the probe,
* so let's not overwrite it with int3 again in this case.
*/
- err = is_optimized(vma->vm_mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized)
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret)
return 0;
}
return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN,
@@ -1090,17 +1086,13 @@ int set_orig_insn(struct arch_uprobe *au
unsigned long vaddr)
{
if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
- struct mm_struct *mm = vma->vm_mm;
- bool optimized = false;
- int err;
-
- err = is_optimized(mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized) {
- err = swbp_unoptimize(auprobe, vma, vaddr);
- WARN_ON_ONCE(err);
- return err;
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret) {
+ ret = swbp_unoptimize(auprobe, vma, vaddr);
+ WARN_ON_ONCE(ret);
+ return ret;
}
}
return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/6] uprobes/x86: Accept more NOP forms
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
2025-08-21 12:28 ` [PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args Peter Zijlstra
2025-08-21 12:28 ` [PATCH 2/6] uprobes/x86: Optimize is_optimize() Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-21 12:28 ` [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack Peter Zijlstra
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
Instead of only accepting the x86_64 nop5 chosen by the kernel, accept
any x86_64 NOP or NOPL instruction that is 5 bytes.
Notably, the x86_64 nop5 pattern is valid in 32bit apps and could get
compiler generated when build for i686 (which introduced NOPL). Since
the trampoline is x86_64 only, make sure to limit to x86_64 code.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1157,10 +1157,37 @@ void arch_uprobe_optimize(struct arch_up
mmap_write_unlock(mm);
}
-static bool can_optimize(struct arch_uprobe *auprobe, unsigned long vaddr)
+static bool insn_is_nop(struct insn *insn)
{
- if (memcmp(&auprobe->insn, x86_nops[5], 5))
+ return insn->opcode.nbytes == 1 && insn->opcode.bytes[0] == 0x90;
+}
+
+static bool insn_is_nopl(struct insn *insn)
+{
+ if (insn->opcode.nbytes != 2)
+ return false;
+
+ if (insn->opcode.bytes[0] != 0x0f || insn->opcode.bytes[1] != 0x1f)
+ return false;
+
+ if (!insn->modrm.nbytes)
+ return false;
+
+ if (X86_MODRM_REG(insn->modrm.bytes[0]) != 0)
+ return false;
+
+ /* 0f 1f /0 - NOPL */
+ return true;
+}
+
+static bool can_optimize(struct insn *insn, unsigned long vaddr)
+{
+ if (!insn->x86_64 || insn->length != 5)
return false;
+
+ if (!insn_is_nop(insn) && !insn_is_nopl(insn))
+ return false;
+
/* We can't do cross page atomic writes yet. */
return PAGE_SIZE - (vaddr & ~PAGE_MASK) >= 5;
}
@@ -1177,7 +1204,7 @@ static void riprel_pre_xol(struct arch_u
static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
}
-static bool can_optimize(struct arch_uprobe *auprobe, unsigned long vaddr)
+static bool can_optimize(struct insn *insn, unsigned long vaddr)
{
return false;
}
@@ -1539,15 +1566,15 @@ static int push_setup_xol_ops(struct arc
*/
int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr)
{
- struct insn insn;
u8 fix_ip_or_call = UPROBE_FIX_IP;
+ struct insn insn;
int ret;
ret = uprobe_init_insn(auprobe, &insn, is_64bit_mm(mm));
if (ret)
return ret;
- if (can_optimize(auprobe, addr))
+ if (can_optimize(&insn, addr))
set_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags);
ret = branch_setup_xol_ops(auprobe, &insn);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
` (2 preceding siblings ...)
2025-08-21 12:28 ` [PATCH 3/6] uprobes/x86: Accept more NOP forms Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-21 18:26 ` Edgecombe, Rick P
2025-08-21 12:28 ` [PATCH 5/6] uprobes/x86: Make asm style consistent Peter Zijlstra
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
The uprobe syscall stores and strips the trampoline stack frame from
the user context, to make it appear similar to an exception at the
original instruction. It then restores the trampoline stack when it
can exit using sysexit.
Make sure to match the regular stack manipulation with shadow stack
operations such that regular and shadow stack don't get out of sync
and causes trouble.
This enables using the optimization when shadow stack is in use.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/shstk.h | 4 ++++
arch/x86/kernel/shstk.c | 40 ++++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/uprobes.c | 17 ++++++++---------
3 files changed, 52 insertions(+), 9 deletions(-)
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -23,6 +23,8 @@ int setup_signal_shadow_stack(struct ksi
int restore_signal_shadow_stack(void);
int shstk_update_last_frame(unsigned long val);
bool shstk_is_enabled(void);
+int shstk_pop(u64 *val);
+int shstk_push(u64 val);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -35,6 +37,8 @@ static inline int setup_signal_shadow_st
static inline int restore_signal_shadow_stack(void) { return 0; }
static inline int shstk_update_last_frame(unsigned long val) { return 0; }
static inline bool shstk_is_enabled(void) { return false; }
+static inline int shstk_pop(u64 *val) { return -ENOTSUPP; }
+static inline int shstk_push(u64 val) { return -ENOTSUPP; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLER__ */
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -246,6 +246,46 @@ static unsigned long get_user_shstk_addr
return ssp;
}
+int shstk_pop(u64 *val)
+{
+ int ret = 0;
+ u64 ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return -ENOTSUPP;
+
+ fpregs_lock_and_load();
+
+ rdmsrq(MSR_IA32_PL3_SSP, ssp);
+ if (val && get_user(*val, (__user u64 *)ssp))
+ ret = -EFAULT;
+ else
+ wrmsrq(MSR_IA32_PL3_SSP, ssp + SS_FRAME_SIZE);
+ fpregs_unlock();
+
+ return ret;
+}
+
+int shstk_push(u64 val)
+{
+ u64 ssp;
+ int ret;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return -ENOTSUPP;
+
+ fpregs_lock_and_load();
+
+ rdmsrq(MSR_IA32_PL3_SSP, ssp);
+ ssp -= SS_FRAME_SIZE;
+ ret = write_user_shstk_64((__user void *)ssp, val);
+ if (!ret)
+ wrmsrq(MSR_IA32_PL3_SSP, ssp);
+ fpregs_unlock();
+
+ return ret;
+}
+
#define SHSTK_DATA_BIT BIT(63)
static int put_shstk_data(u64 __user *addr, u64 data)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -804,7 +804,7 @@ SYSCALL_DEFINE0(uprobe)
{
struct pt_regs *regs = task_pt_regs(current);
struct uprobe_syscall_args args;
- unsigned long ip, sp;
+ unsigned long ip, sp, sret;
int err;
/* Allow execution only from uprobe trampolines. */
@@ -831,6 +831,10 @@ SYSCALL_DEFINE0(uprobe)
sp = regs->sp;
+ err = shstk_pop((u64 *)&sret);
+ if (err == -EFAULT || (!err && sret != args.retaddr))
+ goto sigill;
+
handle_syscall_uprobe(regs, regs->ip);
/*
@@ -855,6 +859,9 @@ SYSCALL_DEFINE0(uprobe)
if (args.retaddr - 5 != regs->ip)
args.retaddr = regs->ip;
+ if (shstk_push(args.retaddr) == -EFAULT)
+ goto sigill;
+
regs->ip = ip;
err = copy_to_user((void __user *)regs->sp, &args, sizeof(args));
@@ -1124,14 +1131,6 @@ void arch_uprobe_optimize(struct arch_up
struct mm_struct *mm = current->mm;
uprobe_opcode_t insn[5];
- /*
- * Do not optimize if shadow stack is enabled, the return address hijack
- * code in arch_uretprobe_hijack_return_addr updates wrong frame when
- * the entry uprobe is optimized and the shadow stack crashes the app.
- */
- if (shstk_is_enabled())
- return;
-
if (!should_optimize(auprobe))
return;
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/6] uprobes/x86: Make asm style consistent
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
` (3 preceding siblings ...)
2025-08-21 12:28 ` [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-21 12:28 ` [PATCH 6/6] uprobes/x86: Add SLS mitigation to the trampolines Peter Zijlstra
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
The asm syntax in uretprobe_trampoline and uprobe_trampoline differs
in the use of operand size suffixes. Make them consistent and remove
all size suffixes.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -321,21 +321,21 @@ asm (
".pushsection .rodata\n"
".global uretprobe_trampoline_entry\n"
"uretprobe_trampoline_entry:\n"
- "pushq %rax\n"
- "pushq %rcx\n"
- "pushq %r11\n"
- "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "push %rax\n"
+ "push %rcx\n"
+ "push %r11\n"
+ "mov $" __stringify(__NR_uretprobe) ", %rax\n"
"syscall\n"
".global uretprobe_syscall_check\n"
"uretprobe_syscall_check:\n"
- "popq %r11\n"
- "popq %rcx\n"
+ "pop %r11\n"
+ "pop %rcx\n"
/*
* The uretprobe syscall replaces stored %rax value with final
* return address, so we don't restore %rax in here and just
* call ret.
*/
- "retq\n"
+ "ret\n"
".global uretprobe_trampoline_end\n"
"uretprobe_trampoline_end:\n"
".popsection\n"
@@ -885,7 +885,7 @@ asm (
"push %rcx\n"
"push %r11\n"
"push %rax\n"
- "movq $" __stringify(__NR_uprobe) ", %rax\n"
+ "mov $" __stringify(__NR_uprobe) ", %rax\n"
"syscall\n"
"pop %rax\n"
"pop %r11\n"
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/6] uprobes/x86: Add SLS mitigation to the trampolines
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
` (4 preceding siblings ...)
2025-08-21 12:28 ` [PATCH 5/6] uprobes/x86: Make asm style consistent Peter Zijlstra
@ 2025-08-21 12:28 ` Peter Zijlstra
2025-08-21 14:18 ` [PATCH 0/6] uprobes/x86: Cleanups and fixes Jiri Olsa
2025-08-22 15:51 ` Oleg Nesterov
7 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-21 12:28 UTC (permalink / raw)
To: jolsa, oleg, andrii, mhiramat
Cc: linux-kernel, peterz, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
It is trivial; no reason not to.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 2 ++
1 file changed, 2 insertions(+)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -336,6 +336,7 @@ asm (
* call ret.
*/
"ret\n"
+ "int3\n"
".global uretprobe_trampoline_end\n"
"uretprobe_trampoline_end:\n"
".popsection\n"
@@ -891,6 +892,7 @@ asm (
"pop %r11\n"
"pop %rcx\n"
"ret\n"
+ "int3\n"
".balign " __stringify(PAGE_SIZE) "\n"
".popsection\n"
);
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
` (5 preceding siblings ...)
2025-08-21 12:28 ` [PATCH 6/6] uprobes/x86: Add SLS mitigation to the trampolines Peter Zijlstra
@ 2025-08-21 14:18 ` Jiri Olsa
2025-08-21 18:27 ` Edgecombe, Rick P
2025-08-22 8:42 ` Jiri Olsa
2025-08-22 15:51 ` Oleg Nesterov
7 siblings, 2 replies; 20+ messages in thread
From: Jiri Olsa @ 2025-08-21 14:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: oleg, andrii, mhiramat, linux-kernel, alx, eyal.birger, kees, bpf,
linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
On Thu, Aug 21, 2025 at 02:28:22PM +0200, Peter Zijlstra wrote:
> Hi,
>
> These are cleanups and fixes that I applied on top of Jiri's patches:
>
> https://lkml.kernel.org/r/20250720112133.244369-1-jolsa@kernel.org
>
> The combined lot sits in:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
>
> Jiri was going to send me some selftest updates that might mean rebasing that
> tree, but we'll see. If this all works we'll land it in -tip.
>
hi,
sent the selftest fix in here:
https://lore.kernel.org/bpf/20250821141557.13233-1-jolsa@kernel.org/T/#u
thanks,
jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack
2025-08-21 12:28 ` [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack Peter Zijlstra
@ 2025-08-21 18:26 ` Edgecombe, Rick P
0 siblings, 0 replies; 20+ messages in thread
From: Edgecombe, Rick P @ 2025-08-21 18:26 UTC (permalink / raw)
To: jolsa@kernel.org, peterz@infradead.org, mhiramat@kernel.org,
andrii@kernel.org, oleg@redhat.com
Cc: songliubraving@fb.com, alx@kernel.org, alan.maguire@oracle.com,
David.Laight@ACULAB.COM, john.fastabend@gmail.com,
linux-kernel@vger.kernel.org, mingo@kernel.org,
rostedt@goodmis.org, yhs@fb.com, eyal.birger@gmail.com,
kees@kernel.org, linux-trace-kernel@vger.kernel.org,
bpf@vger.kernel.org, thomas@t-8ch.de, haoluo@google.com,
x86@kernel.org
On Thu, 2025-08-21 at 14:28 +0200, Peter Zijlstra wrote:
> The uprobe syscall stores and strips the trampoline stack frame from
> the user context, to make it appear similar to an exception at the
> original instruction. It then restores the trampoline stack when it
> can exit using sysexit.
>
> Make sure to match the regular stack manipulation with shadow stack
> operations such that regular and shadow stack don't get out of sync
> and causes trouble.
>
> This enables using the optimization when shadow stack is in use.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/x86/include/asm/shstk.h | 4 ++++
> arch/x86/kernel/shstk.c | 40 ++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/uprobes.c | 17 ++++++++---------
> 3 files changed, 52 insertions(+), 9 deletions(-)
>
> --- a/arch/x86/include/asm/shstk.h
> +++ b/arch/x86/include/asm/shstk.h
> @@ -23,6 +23,8 @@ int setup_signal_shadow_stack(struct ksi
> int restore_signal_shadow_stack(void);
> int shstk_update_last_frame(unsigned long val);
> bool shstk_is_enabled(void);
> +int shstk_pop(u64 *val);
> +int shstk_push(u64 val);
> #else
> static inline long shstk_prctl(struct task_struct *task, int option,
> unsigned long arg2) { return -EINVAL; }
> @@ -35,6 +37,8 @@ static inline int setup_signal_shadow_st
> static inline int restore_signal_shadow_stack(void) { return 0; }
> static inline int shstk_update_last_frame(unsigned long val) { return 0; }
> static inline bool shstk_is_enabled(void) { return false; }
> +static inline int shstk_pop(u64 *val) { return -ENOTSUPP; }
> +static inline int shstk_push(u64 val) { return -ENOTSUPP; }
> #endif /* CONFIG_X86_USER_SHADOW_STACK */
>
> #endif /* __ASSEMBLER__ */
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -246,6 +246,46 @@ static unsigned long get_user_shstk_addr
> return ssp;
> }
>
> +int shstk_pop(u64 *val)
> +{
> + int ret = 0;
> + u64 ssp;
> +
> + if (!features_enabled(ARCH_SHSTK_SHSTK))
> + return -ENOTSUPP;
> +
> + fpregs_lock_and_load();
> +
> + rdmsrq(MSR_IA32_PL3_SSP, ssp);
> + if (val && get_user(*val, (__user u64 *)ssp))
It makes it so shstk_pop() can incssp without pushing anything to the shadow
stack, but nothing uses this.
Also, since there is no read_user_shstk_64() it should probably check that the
VMA is actually shadow stack, like how it does in shstk_pop_sigframe(). What
this actually would expose, I'm not sure. It might be ok. There would just be a
fault later during shstk_push(args.retaddr) I guess.
Hmm, I guess no strong objections, but I'm still not sure it's worth supporting
the optimization.
> + ret = -EFAULT;
> + else
> + wrmsrq(MSR_IA32_PL3_SSP, ssp + SS_FRAME_SIZE);
> + fpregs_unlock();
> +
> + return ret;
> +}
> +
> +int shstk_push(u64 val)
> +{
> + u64 ssp;
> + int ret;
> +
> + if (!features_enabled(ARCH_SHSTK_SHSTK))
> + return -ENOTSUPP;
> +
> + fpregs_lock_and_load();
> +
> + rdmsrq(MSR_IA32_PL3_SSP, ssp);
> + ssp -= SS_FRAME_SIZE;
> + ret = write_user_shstk_64((__user void *)ssp, val);
> + if (!ret)
> + wrmsrq(MSR_IA32_PL3_SSP, ssp);
> + fpregs_unlock();
> +
> + return ret;
> +}
> +
> #define SHSTK_DATA_BIT BIT(63)
>
> static int put_shstk_data(u64 __user *addr, u64 data)
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -804,7 +804,7 @@ SYSCALL_DEFINE0(uprobe)
> {
> struct pt_regs *regs = task_pt_regs(current);
> struct uprobe_syscall_args args;
> - unsigned long ip, sp;
> + unsigned long ip, sp, sret;
> int err;
>
> /* Allow execution only from uprobe trampolines. */
> @@ -831,6 +831,10 @@ SYSCALL_DEFINE0(uprobe)
>
> sp = regs->sp;
>
> + err = shstk_pop((u64 *)&sret);
> + if (err == -EFAULT || (!err && sret != args.retaddr))
> + goto sigill;
> +
> handle_syscall_uprobe(regs, regs->ip);
>
> /*
> @@ -855,6 +859,9 @@ SYSCALL_DEFINE0(uprobe)
> if (args.retaddr - 5 != regs->ip)
> args.retaddr = regs->ip;
>
> + if (shstk_push(args.retaddr) == -EFAULT)
> + goto sigill;
> +
> regs->ip = ip;
>
> err = copy_to_user((void __user *)regs->sp, &args, sizeof(args));
> @@ -1124,14 +1131,6 @@ void arch_uprobe_optimize(struct arch_up
> struct mm_struct *mm = current->mm;
> uprobe_opcode_t insn[5];
>
> - /*
> - * Do not optimize if shadow stack is enabled, the return address hijack
> - * code in arch_uretprobe_hijack_return_addr updates wrong frame when
> - * the entry uprobe is optimized and the shadow stack crashes the app.
> - */
> - if (shstk_is_enabled())
> - return;
> -
> if (!should_optimize(auprobe))
> return;
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 14:18 ` [PATCH 0/6] uprobes/x86: Cleanups and fixes Jiri Olsa
@ 2025-08-21 18:27 ` Edgecombe, Rick P
2025-08-21 19:52 ` Jiri Olsa
2025-08-22 8:42 ` Jiri Olsa
1 sibling, 1 reply; 20+ messages in thread
From: Edgecombe, Rick P @ 2025-08-21 18:27 UTC (permalink / raw)
To: olsajiri@gmail.com, peterz@infradead.org
Cc: songliubraving@fb.com, alx@kernel.org, alan.maguire@oracle.com,
mhiramat@kernel.org, andrii@kernel.org, john.fastabend@gmail.com,
linux-kernel@vger.kernel.org, mingo@kernel.org,
rostedt@goodmis.org, David.Laight@aculab.com, yhs@fb.com,
oleg@redhat.com, kees@kernel.org, eyal.birger@gmail.com,
bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
thomas@t-8ch.de, haoluo@google.com, x86@kernel.org
On Thu, 2025-08-21 at 16:18 +0200, Jiri Olsa wrote:
> hi,
> sent the selftest fix in here:
> https://lore.kernel.org/bpf/20250821141557.13233-1-jolsa@kernel.org/T/#u
I tried for a bit to run the BPF selftests, and ran into various issues. Did you
have access to CET HW to test?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 18:27 ` Edgecombe, Rick P
@ 2025-08-21 19:52 ` Jiri Olsa
2025-08-21 19:57 ` Edgecombe, Rick P
0 siblings, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2025-08-21 19:52 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, peterz@infradead.org, songliubraving@fb.com,
alx@kernel.org, alan.maguire@oracle.com, mhiramat@kernel.org,
andrii@kernel.org, john.fastabend@gmail.com,
linux-kernel@vger.kernel.org, mingo@kernel.org,
rostedt@goodmis.org, David.Laight@aculab.com, yhs@fb.com,
oleg@redhat.com, kees@kernel.org, eyal.birger@gmail.com,
bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
thomas@t-8ch.de, haoluo@google.com, x86@kernel.org
On Thu, Aug 21, 2025 at 06:27:23PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2025-08-21 at 16:18 +0200, Jiri Olsa wrote:
> > hi,
> > sent the selftest fix in here:
> > https://lore.kernel.org/bpf/20250821141557.13233-1-jolsa@kernel.org/T/#u
>
> I tried for a bit to run the BPF selftests, and ran into various issues. Did you
> have access to CET HW to test?
yes,
# cat /proc/cpuinfo | grep user_shstk | head -1
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc art arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf tsc_known_freq pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch cpuid_fault epb ssbd ibrs ibpb stibp ibrs_enhanced tpr_shadow flexpriority ept vpid ept_ad fsgsbase tsc_adjust bmi1 avx2 smep bmi2 erms invpcid rdseed adx smap clflushopt clwb intel_pt sha_ni xsaveopt xsavec xgetbv1 xsaves split_lock_detect user_shstk avx_vnni dtherm ida arat pln pts hwp hwp_notify hwp_act_window hwp_epp hwp_pkg_req hfi vnmi umip pku ospke waitpkg gfni vaes vpclmulqdq rdpid bus_lock_detect movdiri movdir64b fsrm md_clear serialize pconfig arch_lbr ibt flush_l1d arch_capabilities
also I had to merge in bpf-next/master to be able to build it,
what issues did you see?
jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 19:52 ` Jiri Olsa
@ 2025-08-21 19:57 ` Edgecombe, Rick P
0 siblings, 0 replies; 20+ messages in thread
From: Edgecombe, Rick P @ 2025-08-21 19:57 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, alx@kernel.org, alan.maguire@oracle.com,
mhiramat@kernel.org, andrii@kernel.org, john.fastabend@gmail.com,
mingo@kernel.org, linux-kernel@vger.kernel.org,
rostedt@goodmis.org, David.Laight@aculab.com, yhs@fb.com,
oleg@redhat.com, eyal.birger@gmail.com, kees@kernel.org,
peterz@infradead.org, bpf@vger.kernel.org,
linux-trace-kernel@vger.kernel.org, thomas@t-8ch.de,
haoluo@google.com, x86@kernel.org
On Thu, 2025-08-21 at 21:52 +0200, Jiri Olsa wrote:
> yes,
>
> # cat /proc/cpuinfo | grep user_shstk | head -1
> flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc art arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf tsc_known_freq pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch cpuid_fault epb ssbd ibrs ibpb stibp ibrs_enhanced tpr_shadow flexpriority ept vpid ept_ad fsgsbase tsc_adjust bmi1 avx2 smep bmi2 erms invpcid rdseed adx smap clflushopt clwb intel_pt sha_ni xsaveopt xsavec xgetbv1 xsaves split_lock_detect user_shstk avx_vnni dtherm ida arat pln pts hwp hwp_notify hwp_act_window hwp_epp hwp_pkg_req hfi vnmi umip pku ospke waitpkg gfni vaes vpclmulqdq rdpid bus_lock_detect movdiri movdir64b fsrm md_clear serialize pconfig arch_lbr ibt flush_l1d arch_capabilities
>
>
> also I had to merge in bpf-next/master to be able to build it,
> what issues did you see?
>
Ah. I had various compile issues. Some of which were I guess the common issue of
needing the latest tools, but also some others. IIRC some spin lock definitions
were missing. I guess it probably was the lack of merging that branch. But I was
just going to run the test, so if you already have, I'll skip it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 14:18 ` [PATCH 0/6] uprobes/x86: Cleanups and fixes Jiri Olsa
2025-08-21 18:27 ` Edgecombe, Rick P
@ 2025-08-22 8:42 ` Jiri Olsa
2025-08-22 18:05 ` Andrii Nakryiko
1 sibling, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2025-08-22 8:42 UTC (permalink / raw)
To: andrii
Cc: Peter Zijlstra, oleg, mhiramat, Jiri Olsa, linux-kernel, alx,
eyal.birger, kees, bpf, linux-trace-kernel, x86, songliubraving,
yhs, john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Thu, Aug 21, 2025 at 04:18:03PM +0200, Jiri Olsa wrote:
> On Thu, Aug 21, 2025 at 02:28:22PM +0200, Peter Zijlstra wrote:
> > Hi,
> >
> > These are cleanups and fixes that I applied on top of Jiri's patches:
> >
> > https://lkml.kernel.org/r/20250720112133.244369-1-jolsa@kernel.org
> >
> > The combined lot sits in:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
> >
> > Jiri was going to send me some selftest updates that might mean rebasing that
> > tree, but we'll see. If this all works we'll land it in -tip.
> >
>
> hi,
> sent the selftest fix in here:
> https://lore.kernel.org/bpf/20250821141557.13233-1-jolsa@kernel.org/T/#u
Andrii,
do we want any special logistic for the bpf/selftest changes or it could
go through the tip tree?
thanks,
jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
` (6 preceding siblings ...)
2025-08-21 14:18 ` [PATCH 0/6] uprobes/x86: Cleanups and fixes Jiri Olsa
@ 2025-08-22 15:51 ` Oleg Nesterov
7 siblings, 0 replies; 20+ messages in thread
From: Oleg Nesterov @ 2025-08-22 15:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jolsa, andrii, mhiramat, linux-kernel, alx, eyal.birger, kees,
bpf, linux-trace-kernel, x86, songliubraving, yhs, john.fastabend,
haoluo, rostedt, alan.maguire, David.Laight, thomas, mingo,
rick.p.edgecombe
On 08/21, Peter Zijlstra wrote:
>
> These are cleanups and fixes that I applied on top of Jiri's patches:
>
> https://lkml.kernel.org/r/20250720112133.244369-1-jolsa@kernel.org
Can't review 4/6 due to the lack of knowledge.
Other changes look good to me, FWIW
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes
2025-08-22 8:42 ` Jiri Olsa
@ 2025-08-22 18:05 ` Andrii Nakryiko
0 siblings, 0 replies; 20+ messages in thread
From: Andrii Nakryiko @ 2025-08-22 18:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: andrii, Peter Zijlstra, oleg, mhiramat, linux-kernel, alx,
eyal.birger, kees, bpf, linux-trace-kernel, x86, songliubraving,
yhs, john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Fri, Aug 22, 2025 at 1:42 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Aug 21, 2025 at 04:18:03PM +0200, Jiri Olsa wrote:
> > On Thu, Aug 21, 2025 at 02:28:22PM +0200, Peter Zijlstra wrote:
> > > Hi,
> > >
> > > These are cleanups and fixes that I applied on top of Jiri's patches:
> > >
> > > https://lkml.kernel.org/r/20250720112133.244369-1-jolsa@kernel.org
> > >
> > > The combined lot sits in:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
> > >
> > > Jiri was going to send me some selftest updates that might mean rebasing that
> > > tree, but we'll see. If this all works we'll land it in -tip.
> > >
> >
> > hi,
> > sent the selftest fix in here:
> > https://lore.kernel.org/bpf/20250821141557.13233-1-jolsa@kernel.org/T/#u
>
> Andrii,
> do we want any special logistic for the bpf/selftest changes or it could
> go through the tip tree?
let's route selftest changes through tip together with the rest of
uprobe changes, it's unlikely to conflict
Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> thanks,
> jirka
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-21 12:28 ` [PATCH 2/6] uprobes/x86: Optimize is_optimize() Peter Zijlstra
@ 2025-08-26 5:51 ` David Laight
2025-08-26 8:18 ` Peter Zijlstra
2025-08-26 8:25 ` Jiri Olsa
0 siblings, 2 replies; 20+ messages in thread
From: David Laight @ 2025-08-26 5:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jolsa, oleg, andrii, mhiramat, linux-kernel, alx, eyal.birger,
kees, bpf, linux-trace-kernel, x86, songliubraving, yhs,
john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Thu, 21 Aug 2025 14:28:24 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> Make is_optimized() return a tri-state and avoid return through
> argument. This simplifies things a little.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> }
>
> -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> {
> uprobe_opcode_t insn[5];
> int err;
> @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> err = copy_from_vaddr(mm, vaddr, &insn, 5);
> if (err)
> return err;
> - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> - return 0;
> + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> }
>
> static bool should_optimize(struct arch_uprobe *auprobe)
> @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> unsigned long vaddr)
> {
> if (should_optimize(auprobe)) {
> - bool optimized = false;
> - int err;
> -
> /*
> * We could race with another thread that already optimized the probe,
> * so let's not overwrite it with int3 again in this case.
> */
> - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> - if (err)
> - return err;
> - if (optimized)
> + int ret = is_optimized(vma->vm_mm, vaddr);
> + if (ret < 0)
> + return ret;
> + if (ret)
> return 0;
Looks like you should swap over 0 and 1.
That would then be: if (ret <= 0) return ret;
David
> }
> return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN,
> @@ -1090,17 +1086,13 @@ int set_orig_insn(struct arch_uprobe *au
> unsigned long vaddr)
> {
> if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
> - struct mm_struct *mm = vma->vm_mm;
> - bool optimized = false;
> - int err;
> -
> - err = is_optimized(mm, vaddr, &optimized);
> - if (err)
> - return err;
> - if (optimized) {
> - err = swbp_unoptimize(auprobe, vma, vaddr);
> - WARN_ON_ONCE(err);
> - return err;
> + int ret = is_optimized(vma->vm_mm, vaddr);
> + if (ret < 0)
> + return ret;
> + if (ret) {
> + ret = swbp_unoptimize(auprobe, vma, vaddr);
> + WARN_ON_ONCE(ret);
> + return ret;
> }
> }
> return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-26 5:51 ` David Laight
@ 2025-08-26 8:18 ` Peter Zijlstra
2025-08-27 19:32 ` David Laight
2025-08-26 8:25 ` Jiri Olsa
1 sibling, 1 reply; 20+ messages in thread
From: Peter Zijlstra @ 2025-08-26 8:18 UTC (permalink / raw)
To: David Laight
Cc: jolsa, oleg, andrii, mhiramat, linux-kernel, alx, eyal.birger,
kees, bpf, linux-trace-kernel, x86, songliubraving, yhs,
john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > unsigned long vaddr)
> > {
> > if (should_optimize(auprobe)) {
> > - bool optimized = false;
> > - int err;
> > -
> > /*
> > * We could race with another thread that already optimized the probe,
> > * so let's not overwrite it with int3 again in this case.
> > */
> > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > - if (err)
> > - return err;
> > - if (optimized)
> > + int ret = is_optimized(vma->vm_mm, vaddr);
> > + if (ret < 0)
> > + return ret;
> > + if (ret)
> > return 0;
>
> Looks like you should swap over 0 and 1.
> That would then be: if (ret <= 0) return ret;
I considered that, but that was actually more confusing. Yes the return
check is neat, but urgh.
The tri-state return is:
<0 -- error
0 -- false
1 -- true
and that is converted to the 'normal' convention:
<0 -- error
0 -- success
Making that intermediate:
<0 -- error
0 -- true
1 -- false
is just asking for trouble later.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-26 5:51 ` David Laight
2025-08-26 8:18 ` Peter Zijlstra
@ 2025-08-26 8:25 ` Jiri Olsa
2025-08-26 8:33 ` Jiri Olsa
1 sibling, 1 reply; 20+ messages in thread
From: Jiri Olsa @ 2025-08-26 8:25 UTC (permalink / raw)
To: David Laight
Cc: Peter Zijlstra, oleg, andrii, mhiramat, linux-kernel, alx,
eyal.birger, kees, bpf, linux-trace-kernel, x86, songliubraving,
yhs, john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> On Thu, 21 Aug 2025 14:28:24 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > Make is_optimized() return a tri-state and avoid return through
> > argument. This simplifies things a little.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> > 1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> > return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> > }
> >
> > -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> > +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> > {
> > uprobe_opcode_t insn[5];
> > int err;
> > @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> > err = copy_from_vaddr(mm, vaddr, &insn, 5);
> > if (err)
> > return err;
> > - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > - return 0;
> > + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > }
> >
> > static bool should_optimize(struct arch_uprobe *auprobe)
> > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > unsigned long vaddr)
> > {
> > if (should_optimize(auprobe)) {
> > - bool optimized = false;
> > - int err;
> > -
> > /*
> > * We could race with another thread that already optimized the probe,
> > * so let's not overwrite it with int3 again in this case.
> > */
> > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > - if (err)
> > - return err;
> > - if (optimized)
> > + int ret = is_optimized(vma->vm_mm, vaddr);
> > + if (ret < 0)
> > + return ret;
> > + if (ret)
> > return 0;
>
> Looks like you should swap over 0 and 1.
> That would then be: if (ret <= 0) return ret;
hum, but if it's not optimized (ret == 0) we need to follow up with
installing breakpoint through following uprobe_write_opcode call
also I noticed we mix int/bool return, perhaps we could do fix below
jirka
---
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0a8c0a4a5423..853abb2a5638 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1064,7 +1064,7 @@ static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
err = copy_from_vaddr(mm, vaddr, &insn, 5);
if (err)
return err;
- return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
+ return __is_optimized((uprobe_opcode_t *)&insn, vaddr) ? 1 : 0;
}
static bool should_optimize(struct arch_uprobe *auprobe)
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-26 8:25 ` Jiri Olsa
@ 2025-08-26 8:33 ` Jiri Olsa
0 siblings, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2025-08-26 8:33 UTC (permalink / raw)
To: Jiri Olsa
Cc: David Laight, Peter Zijlstra, oleg, andrii, mhiramat,
linux-kernel, alx, eyal.birger, kees, bpf, linux-trace-kernel,
x86, songliubraving, yhs, john.fastabend, haoluo, rostedt,
alan.maguire, David.Laight, thomas, mingo, rick.p.edgecombe
On Tue, Aug 26, 2025 at 10:25:29AM +0200, Jiri Olsa wrote:
> On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> > On Thu, 21 Aug 2025 14:28:24 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > Make is_optimized() return a tri-state and avoid return through
> > > argument. This simplifies things a little.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> > > 1 file changed, 13 insertions(+), 21 deletions(-)
> > >
> > > --- a/arch/x86/kernel/uprobes.c
> > > +++ b/arch/x86/kernel/uprobes.c
> > > @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> > > return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> > > }
> > >
> > > -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> > > +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> > > {
> > > uprobe_opcode_t insn[5];
> > > int err;
> > > @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> > > err = copy_from_vaddr(mm, vaddr, &insn, 5);
> > > if (err)
> > > return err;
> > > - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > > - return 0;
> > > + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > > }
> > >
> > > static bool should_optimize(struct arch_uprobe *auprobe)
> > > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > > unsigned long vaddr)
> > > {
> > > if (should_optimize(auprobe)) {
> > > - bool optimized = false;
> > > - int err;
> > > -
> > > /*
> > > * We could race with another thread that already optimized the probe,
> > > * so let's not overwrite it with int3 again in this case.
> > > */
> > > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > > - if (err)
> > > - return err;
> > > - if (optimized)
> > > + int ret = is_optimized(vma->vm_mm, vaddr);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret)
> > > return 0;
> >
> > Looks like you should swap over 0 and 1.
> > That would then be: if (ret <= 0) return ret;
>
> hum, but if it's not optimized (ret == 0) we need to follow up with
> installing breakpoint through following uprobe_write_opcode call
ah u meant to swap the whole thing.. got it
>
> also I noticed we mix int/bool return, perhaps we could do fix below
>
> jirka
>
>
> ---
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 0a8c0a4a5423..853abb2a5638 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -1064,7 +1064,7 @@ static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> err = copy_from_vaddr(mm, vaddr, &insn, 5);
> if (err)
> return err;
> - return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> + return __is_optimized((uprobe_opcode_t *)&insn, vaddr) ? 1 : 0;
> }
>
> static bool should_optimize(struct arch_uprobe *auprobe)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()
2025-08-26 8:18 ` Peter Zijlstra
@ 2025-08-27 19:32 ` David Laight
0 siblings, 0 replies; 20+ messages in thread
From: David Laight @ 2025-08-27 19:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jolsa, oleg, andrii, mhiramat, linux-kernel, alx, eyal.birger,
kees, bpf, linux-trace-kernel, x86, songliubraving, yhs,
john.fastabend, haoluo, rostedt, alan.maguire, David.Laight,
thomas, mingo, rick.p.edgecombe
On Tue, 26 Aug 2025 10:18:40 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
>
> > > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > > unsigned long vaddr)
> > > {
> > > if (should_optimize(auprobe)) {
> > > - bool optimized = false;
> > > - int err;
> > > -
> > > /*
> > > * We could race with another thread that already optimized the probe,
> > > * so let's not overwrite it with int3 again in this case.
> > > */
> > > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > > - if (err)
> > > - return err;
> > > - if (optimized)
> > > + int ret = is_optimized(vma->vm_mm, vaddr);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret)
> > > return 0;
> >
> > Looks like you should swap over 0 and 1.
> > That would then be: if (ret <= 0) return ret;
>
> I considered that, but that was actually more confusing. Yes the return
> check is neat, but urgh.
>
> The tri-state return is:
>
> <0 -- error
> 0 -- false
> 1 -- true
>
> and that is converted to the 'normal' convention:
>
> <0 -- error
> 0 -- success
>
>
> Making that intermediate:
>
> <0 -- error
> 0 -- true
> 1 -- false
>
> is just asking for trouble later.
I'm sure the function name could be changed to make it all work :-)
David
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2025-08-27 19:33 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-21 12:28 [PATCH 0/6] uprobes/x86: Cleanups and fixes Peter Zijlstra
2025-08-21 12:28 ` [PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args Peter Zijlstra
2025-08-21 12:28 ` [PATCH 2/6] uprobes/x86: Optimize is_optimize() Peter Zijlstra
2025-08-26 5:51 ` David Laight
2025-08-26 8:18 ` Peter Zijlstra
2025-08-27 19:32 ` David Laight
2025-08-26 8:25 ` Jiri Olsa
2025-08-26 8:33 ` Jiri Olsa
2025-08-21 12:28 ` [PATCH 3/6] uprobes/x86: Accept more NOP forms Peter Zijlstra
2025-08-21 12:28 ` [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack Peter Zijlstra
2025-08-21 18:26 ` Edgecombe, Rick P
2025-08-21 12:28 ` [PATCH 5/6] uprobes/x86: Make asm style consistent Peter Zijlstra
2025-08-21 12:28 ` [PATCH 6/6] uprobes/x86: Add SLS mitigation to the trampolines Peter Zijlstra
2025-08-21 14:18 ` [PATCH 0/6] uprobes/x86: Cleanups and fixes Jiri Olsa
2025-08-21 18:27 ` Edgecombe, Rick P
2025-08-21 19:52 ` Jiri Olsa
2025-08-21 19:57 ` Edgecombe, Rick P
2025-08-22 8:42 ` Jiri Olsa
2025-08-22 18:05 ` Andrii Nakryiko
2025-08-22 15:51 ` Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).