The following commit has been merged into the x86/core branch of tip:

Commit-ID:     a17e1039874831c49d3c15d699a65a8f8130dbb5
Gitweb:        https://git.kernel.org/tip/a17e1039874831c49d3c15d699a65a8f8130dbb5
Author:        Juergen Gross <jgross@suse.com>
AuthorDate:    Mon, 29 Sep 2025 13:29:47 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 14 Oct 2025 10:38:11 +02:00

x86/alternative: Patch a single alternative location only once

Instead of patching a single location potentially multiple times in
case of nested ALTERNATIVE()s, do the patching only after having
evaluated all alt_instr instances for that location.

This has multiple advantages:

- In case of replacing an indirect with a direct call using the
  ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
  instance before any other instances at the same location (the
  original instruction is needed for finding the target of the direct
  call).

- In case of nested ALTERNATIVE()s there is no intermediate replacement
  visible. This avoids any problems in case e.g. an interrupt is
  happening between the single instances and the patched location is
  used during handling the interrupt.

Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/alternative.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d86a012..90c0d16 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 	u8 insn_buff[MAX_PATCH_LEN];
 	u8 *instr;
 	struct alt_instr *a, *b;
+	unsigned int instances = 0;
+	bool patched = false;
 
 	DPRINTK(ALT, "alt table %px, -> %px", start, end);
 
@@ -677,9 +679,13 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * padding for all alt_instr entries for this site (nested
 		 * alternatives result in consecutive entries).
 		 */
-		for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
-			u8 len = max(a->instrlen, b->instrlen);
-			a->instrlen = b->instrlen = len;
+		if (!instances) {
+			for (b = a+1; b < end && instr_va(b) == instr_va(a); b++) {
+				u8 len = max(a->instrlen, b->instrlen);
+				a->instrlen = b->instrlen = len;
+			}
+			instances = b - a;
+			patched = false;
 		}
 
 		instr = instr_va(a);
@@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
 		 * - feature not present but ALT_FLAG_NOT is set to mean,
 		 *   patch if feature is *NOT* present.
 		 */
-		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
-			memcpy(insn_buff, instr, a->instrlen);
-			optimize_nops(instr, insn_buff, a->instrlen);
-		} else {
+		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
 			apply_one_alternative(instr, insn_buff, a);
+			patched = true;
 		}
 
-		text_poke_early(instr, insn_buff, a->instrlen);
+		instances--;
+		if (!instances) {
+			if (!patched) {
+				memcpy(insn_buff, instr, a->instrlen);
+				optimize_nops(instr, insn_buff, a->instrlen);
+			}
+			text_poke_early(instr, insn_buff, a->instrlen);
+		}
 	}
 
 	kasan_enable_current();

On Tue, Oct 14, 2025 at 08:42:34AM -0000, tip-bot2 for Juergen Gross wrote:
> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  	u8 insn_buff[MAX_PATCH_LEN];
>  	u8 *instr;
>  	struct alt_instr *a, *b;
> +	unsigned int instances = 0;
> +	bool patched = false;

Except that we have the reverse fir tree rule in tip for function-local vars.

The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>  		 * - feature not present but ALT_FLAG_NOT is set to mean,
>  		 *   patch if feature is *NOT* present.
>  		 */
> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> -			memcpy(insn_buff, instr, a->instrlen);
> -			optimize_nops(instr, insn_buff, a->instrlen);
> -		} else {
> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>  			apply_one_alternative(instr, insn_buff, a);
> +			patched = true;
>  		}
>  
> -		text_poke_early(instr, insn_buff, a->instrlen);
> +		instances--;
> +		if (!instances) {
> +			if (!patched) {

I don't see how this is making this code better - this is slowly turning into
an unreadable mess with those magic "instances" and "patched".

And frankly, the justification for this patch is also meh: an interrupt might
use the location?!? If this is a real issue then we better disable IRQs around
it. But not make the code yucky.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 14.10.25 14:59, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 08:42:34AM -0000, tip-bot2 for Juergen Gross wrote:
>> @@ -648,6 +648,8 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   	u8 insn_buff[MAX_PATCH_LEN];
>>   	u8 *instr;
>>   	struct alt_instr *a, *b;
>> +	unsigned int instances = 0;
>> +	bool patched = false;
> 
> Except that we have the reverse fir tree rule in tip for function-local vars.

Okay.

>> @@ -692,14 +698,19 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>   		 * - feature not present but ALT_FLAG_NOT is set to mean,
>>   		 *   patch if feature is *NOT* present.
>>   		 */
>> -		if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
>> -			memcpy(insn_buff, instr, a->instrlen);
>> -			optimize_nops(instr, insn_buff, a->instrlen);
>> -		} else {
>> +		if (!boot_cpu_has(a->cpuid) != !(a->flags & ALT_FLAG_NOT)) {
>>   			apply_one_alternative(instr, insn_buff, a);
>> +			patched = true;
>>   		}
>>   
>> -		text_poke_early(instr, insn_buff, a->instrlen);
>> +		instances--;
>> +		if (!instances) {
>> +			if (!patched) {
> 
> I don't see how this is making this code better - this is slowly turning into
> an unreadable mess with those magic "instances" and "patched".
> 
> And frankly, the justification for this patch is also meh: an interrupt might
> use the location?!? If this is a real issue then we better disable IRQs around
> it. But not make the code yucky.

For one it is not the only justification.

And just for the record: NMI handling is using ALTERNATIVE_2, so you
can't just "disable interrupts" and be sure it will work.


Juergen

On Tue, Oct 14, 2025 at 04:08:47PM +0200, Jürgen Groß wrote:
> And just for the record: NMI handling is using ALTERNATIVE_2, so you
> can't just "disable interrupts" and be sure it will work.

Are you fixing anything you're actually hitting while patching alternatives or
are you talking hypothetically here?

Because I can't think of an easy way to trigger NMIs that early during boot,
when not even SMP is up.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 14.10.25 16:12, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:08:47PM +0200, Jürgen Groß wrote:
>> And just for the record: NMI handling is using ALTERNATIVE_2, so you
>> can't just "disable interrupts" and be sure it will work.
> 
> Are you fixing anything you're actually hitting while patching alternatives or
> are you talking hypothetically here?

I really ran into issues while writing my paravirt MSR series, as I had
an ALTERNATIVE3() invocation modifying the original instruction (the
indirect paravirt call) with an WRMSR and then trying to turn the no
longer existing indirect call into a direct one.

So I didn't have the intermediate issue in interrupt handling, but the
issue with rules how to place indirect call replacements in ALTERNATIVEn()
invocations.

The interrupt handling still might be a thing, while NMI is for sure rather
unlikely.

> 
> Because I can't think of an easy way to trigger NMIs that early during boot,
> when not even SMP is up.
> 

NMIs are probably really a more theoretical issue. And even if NMIs are
affected, they will still be affected in case they happen just when half of
the new instruction bytes have been written.


Juergen

On Tue, Oct 14, 2025 at 04:22:09PM +0200, Jürgen Groß wrote:
> I really ran into issues while writing my paravirt MSR series, as I had
> an ALTERNATIVE3() invocation modifying the original instruction (the
> indirect paravirt call) with an WRMSR and then trying to turn the no
> longer existing indirect call into a direct one.

So put *that* in the commit message. Along with a detailed example of what
you were seeing. This is bazillion miles more useful than some hypothetically,
potentially ... case.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 14.10.25 16:26, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:22:09PM +0200, Jürgen Groß wrote:
>> I really ran into issues while writing my paravirt MSR series, as I had
>> an ALTERNATIVE3() invocation modifying the original instruction (the
>> indirect paravirt call) with an WRMSR and then trying to turn the no
>> longer existing indirect call into a direct one.
> 
> So put *that* in the commit message. Along with a detailed example of what
> you were seeing. This is bazillion miles more useful than some hypothetically,
> potentially ... case.

I have this here in the commit message:

   - In case of replacing an indirect with a direct call using the
     ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
     instance before any other instances at the same location (the
     original instruction is needed for finding the target of the direct
     call).

which is explaining why the problem is occurring. Isn't that enough?


Juergen

On Tue, Oct 14, 2025 at 04:31:31PM +0200, Jürgen Groß wrote:
> I have this here in the commit message:
> 
>   - In case of replacing an indirect with a direct call using the
>     ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>     instance before any other instances at the same location (the
>     original instruction is needed for finding the target of the direct
>     call).
> 
> which is explaining why the problem is occurring. Isn't that enough?

I can guess what this is about but a concrete example here would make it
a lot clearer, I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 14.10.25 16:39, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 04:31:31PM +0200, Jürgen Groß wrote:
>> I have this here in the commit message:
>>
>>    - In case of replacing an indirect with a direct call using the
>>      ALT_FLAG_DIRECT_CALL flag, there is no longer the need to have that
>>      instance before any other instances at the same location (the
>>      original instruction is needed for finding the target of the direct
>>      call).
>>
>> which is explaining why the problem is occurring. Isn't that enough?
> 
> I can guess what this is about but a concrete example here would make it
> a lot clearer, I'd say.

Okay, I'll expand that with the concrete example.


Juergen

On Tue, Oct 14, 2025 at 02:59:09PM +0200, Borislav Petkov wrote:
> And frankly, the justification for this patch is also meh: an interrupt might
> use the location?!? If this is a real issue then we better disable IRQs around
> it. But not make the code yucky.

And this patch is not doing what it is claiming is doing:

Here's an incarnation of XSTATE_XSAVE which is a 3-way alternative:

At location ffffffff81283cd3, it replaces the default XSAVE with XSAVEOPT.

[    2.456696] SMP alternatives: feat: 10*32+0, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6d9, len: 6) flags: 0x0
[    2.459317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40	xsave64 0x40(%r12)
[    2.460806] SMP alternatives: ffffffff89c1e6d9:   rpl_insn: 49 0f ae 74 24 40	xsaveopt64 0x40(%r12)
[    2.463316] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f ae 74 24 40

and then that exact same location:

[    2.464757] SMP alternatives: feat: 10*32+1, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6df, len: 6) flags: 0x0
[    2.467317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40
[    2.468746] SMP alternatives: ffffffff89c1e6df:   rpl_insn: 49 0f c7 64 24 40	xsaves64 0x40(%r12)
[    2.470167] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f c7 64 24 40

gets XSAVES.

So, long story short, this needs more thought:

1. check whether patching is needed

2. a helper function evaluates all instances and figures out the final insn
   bytes which need to be patched along with the proper padding

3. the proper bytes are copied into the target location and all good

But not like this - the idea is somewhat ok but it needs to be executed in
a cleaner manner.

I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 14.10.25 15:25, Borislav Petkov wrote:
> On Tue, Oct 14, 2025 at 02:59:09PM +0200, Borislav Petkov wrote:
>> And frankly, the justification for this patch is also meh: an interrupt might
>> use the location?!? If this is a real issue then we better disable IRQs around
>> it. But not make the code yucky.
> 
> And this patch is not doing what it is claiming is doing:
> 
> Here's an incarnation of XSTATE_XSAVE which is a 3-way alternative:
> 
> At location ffffffff81283cd3, it replaces the default XSAVE with XSAVEOPT.
> 
> [    2.456696] SMP alternatives: feat: 10*32+0, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6d9, len: 6) flags: 0x0
> [    2.459317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40	xsave64 0x40(%r12)
> [    2.460806] SMP alternatives: ffffffff89c1e6d9:   rpl_insn: 49 0f ae 74 24 40	xsaveopt64 0x40(%r12)
> [    2.463316] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f ae 74 24 40
> 
> and then that exact same location:
> 
> [    2.464757] SMP alternatives: feat: 10*32+1, old: (save_fpregs_to_fpstate+0x43/0xa0 (ffffffff81283cd3) len: 6), repl: (ffffffff89c1e6df, len: 6) flags: 0x0
> [    2.467317] SMP alternatives: ffffffff81283cd3:   old_insn: 49 0f ae 64 24 40
> [    2.468746] SMP alternatives: ffffffff89c1e6df:   rpl_insn: 49 0f c7 64 24 40	xsaves64 0x40(%r12)
> [    2.470167] SMP alternatives: ffffffff81283cd3: final_insn: 49 0f c7 64 24 40
> 
> gets XSAVES.

Oh, indeed, I should have adapted the printing. The patching is fine,
it is just the debug output which claims to have written 2 different
instructions to old_insn, while it didn't.

> 
> So, long story short, this needs more thought:
> 
> 1. check whether patching is needed
> 
> 2. a helper function evaluates all instances and figures out the final insn
>     bytes which need to be patched along with the proper padding
> 
> 3. the proper bytes are copied into the target location and all good
> 
> But not like this - the idea is somewhat ok but it needs to be executed in
> a cleaner manner.
> 
> I'd say.

I can have a try with this approach.

Could take some time, vacation is coming up...


Juergen

On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:

> Could take some time, vacation is coming up...

Enjoy! This patch has meanwhile gone away.

On 14.10.25 16:18, Peter Zijlstra wrote:
> On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:
> 
>> Could take some time, vacation is coming up...
> 
> Enjoy! This patch has meanwhile gone away.
> 

I'm not sure the "x86/alternative: Refactor apply_alternatives()"
patch will stay the way it is now. Maybe better to remove it, too.

"x86/alternative: Drop not needed test after call of alt_replace_call()"
won't be changed, so it can be kept IMHO.


Juergen

On Tue, Oct 14, 2025 at 04:26:07PM +0200, Juergen Gross wrote:
> I'm not sure the "x86/alternative: Refactor apply_alternatives()"
> patch will stay the way it is now. Maybe better to remove it, too.

Ok, lemme zap it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On Tue, Oct 14, 2025 at 04:13:11PM +0200, Jürgen Groß wrote:
> I can have a try with this approach.
> 
> Could take some time, vacation is coming up...

Thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette