[PATCH 0/6] uprobes/x86: Cleanups and fixes

[PATCH 0/6] uprobes/x86: Cleanups and fixes

[Peter Zijlstra]

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.

[PATCH 1/6] uprobes/x86: Add struct uretprobe_syscall_args

[Peter Zijlstra]

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;
 

[PATCH 2/6] uprobes/x86: Optimize is_optimize()

[Peter Zijlstra]

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,

Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()

[David Laight]

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,
> 
> 
> 

Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()

[Peter Zijlstra]

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.

Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()

[David Laight]

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

Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()

[Jiri Olsa]

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)

Re: [PATCH 2/6] uprobes/x86: Optimize is_optimize()

[Jiri Olsa]

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)

[PATCH 3/6] uprobes/x86: Accept more NOP forms

[Peter Zijlstra]

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);

[PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack

[Peter Zijlstra]

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;
 

Re: [PATCH 4/6] uprobes/x86: Fix uprobe syscall vs shadow stack

[Edgecombe, Rick P]

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;
>  
> 
> 

[PATCH 5/6] uprobes/x86: Make asm style consistent

[Peter Zijlstra]

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"

[PATCH 6/6] uprobes/x86: Add SLS mitigation to the trampolines

[Peter Zijlstra]

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"
 );

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Jiri Olsa]

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

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Edgecombe, Rick P]

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?

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Jiri Olsa]

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

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Edgecombe, Rick P]

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.

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Jiri Olsa]

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

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Andrii Nakryiko]

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

Re: [PATCH 0/6] uprobes/x86: Cleanups and fixes

[Oleg Nesterov]

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>