[PATCH 0/1] x86/fpu: Allow clearcpuid= to clear several bits

[PATCH 0/1] x86/fpu: Allow clearcpuid= to clear several bits

[John Haxby]

Back in the mists of time, well, prior to 4.14.9, you could put clearcpuid=N
several times on the command line to clear several CPUID bits.  Unfortunately,
there was a problem with the way that this was done which meant XSAVE, in
particular, wasn't cleared early enough.  This was fixed in 0c2a3913d6f5
("x86/fpu: Parse clearcpuid= as early XSAVE argument").  Unfortunately that
also meant that only one bit could be cleared.

This patch mostly fixes that.  Gone is the old, anachronistic, multiple
clearcpuid= arguments to be replaced by a single clearcpuid=BITNUM[,BITNUM,...]
argument.  It's no longer possible to clear perhaps dozens of flags with a very
long kernel command line, but you can clear up to about eight bits now instead
of just one.

jch


John Haxby (1):
  x86/fpu: Allow clearcpuid= to clear several bits

 .../admin-guide/kernel-parameters.txt         | 24 ++++++++++---------
 arch/x86/kernel/fpu/init.c                    | 18 ++++++++------
 2 files changed, 24 insertions(+), 18 deletions(-)

-- 
2.25.3

[PATCH 1/1] x86/fpu: Allow clearcpuid= to clear several bits

[John Haxby]

Prior to 0c2a3913d6f5, clearcpuid= could be specified several times on
the command line to clear several bits. The old multiple option is a
little anachronistic so change clearcpuid to accept a comma-separated
list of numbers. Up to about eight bits can be cleared.

Fixes: 0c2a3913d6f5 ("x86/fpu: Parse clearcpuid= as early XSAVE argument")
Signed-off-by: John Haxby <john.haxby@oracle.com>
Cc: stable@vger.kernel.org
---
 .../admin-guide/kernel-parameters.txt         | 24 ++++++++++---------
 arch/x86/kernel/fpu/init.c                    | 18 ++++++++------
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f2a93c8679e8..f380781be9e0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -577,18 +577,20 @@
 			loops can be debugged more effectively on production
 			systems.
 
-	clearcpuid=BITNUM [X86]
-			Disable CPUID feature X for the kernel. See
+	clearcpuid=BITNUM[,BITNUM,...] [X86]
+			Disable CPUID features for the kernel. See
 			arch/x86/include/asm/cpufeatures.h for the valid bit
-			numbers. Note the Linux specific bits are not necessarily
-			stable over kernel options, but the vendor specific
-			ones should be.
-			Also note that user programs calling CPUID directly
-			or using the feature without checking anything
-			will still see it. This just prevents it from
-			being used by the kernel or shown in /proc/cpuinfo.
-			Also note the kernel might malfunction if you disable
-			some critical bits.
+			numbers. Up to about eight bits can be cleared. Note the
+			Linux specific bits are not necessarily stable over
+			kernel options, but the vendor specific ones should be.
+			Also note that user programs calling CPUID directly or
+			using the feature without checking anything will still
+			see it. This just prevents it from being used by the
+			kernel or shown in /proc/cpuinfo.  Also note the kernel
+			might malfunction if you disable some critical bits.
+			Consider using a virtual machine emulating an older CPU
+			type for clearing many bits or for making the cleared
+			bits visible to user programs.
 
 	cma=nn[MG]@[start[MG][-end[MG]]]
 			[ARM,X86,KNL]
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 6ce7e0a23268..8d826505c22e 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -243,8 +243,6 @@ static void __init fpu__init_system_ctx_switch(void)
 static void __init fpu__init_parse_early_param(void)
 {
 	char arg[32];
-	char *argptr = arg;
-	int bit;
 
 #ifdef CONFIG_X86_32
 	if (cmdline_find_option_bool(boot_command_line, "no387"))
@@ -268,11 +266,17 @@ static void __init fpu__init_parse_early_param(void)
 		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 
 	if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
-				sizeof(arg)) &&
-	    get_option(&argptr, &bit) &&
-	    bit >= 0 &&
-	    bit < NCAPINTS * 32)
-		setup_clear_cpu_cap(bit);
+				sizeof(arg))) {
+		/* cpuid bit numbers are mostly three digits */
+		enum  { nints = sizeof(arg)/(3+1) + 1 };
+		int i, bits[nints];
+
+		get_options(arg, nints, bits);
+		for (i = 1; i <= bits[0]; i++) {
+			if (bits[i] >= 0 && bits[i] < NCAPINTS * 32)
+				setup_clear_cpu_cap(bits[i]);
+		}
+	}
 }
 
 /*
-- 
2.25.3

Re: [PATCH 1/1] x86/fpu: Allow clearcpuid= to clear several bits

[Andi Kleen]

Thanks good catch.

>  	if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
> -				sizeof(arg)) &&
> -	    get_option(&argptr, &bit) &&
> -	    bit >= 0 &&
> -	    bit < NCAPINTS * 32)
> -		setup_clear_cpu_cap(bit);
> +				sizeof(arg))) {
> +		/* cpuid bit numbers are mostly three digits */
> +		enum  { nints = sizeof(arg)/(3+1) + 1 };

Not sure what the digits have to do with the stack space of an int array.

We should have enough stack to afford some more than 8.

Would be good to have a warning if the arguments are longer.

Maybe it would be simpler to fix the early arg parser
to allow multiple instances again? That would also avoid the limit,
and keep everything compatible.

-Andi


> +		int i, bits[nints];
> +
> +		get_options(arg, nints, bits);
> +		for (i = 1; i <= bits[0]; i++) {
> +			if (bits[i] >= 0 && bits[i] < NCAPINTS * 32)
> +				setup_clear_cpu_cap(bits[i]);
> +		}
> +	}
>  }
>  
>  /*
> -- 
> 2.25.3
> 

Re: [PATCH 1/1] x86/fpu: Allow clearcpuid= to clear several bits

[John Haxby]

> On 22 Apr 2020, at 15:35, Andi Kleen <ak@linux.intel.com> wrote:
> 
> 
> Thanks good catch.
> 
>> 	if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
>> -				sizeof(arg)) &&
>> -	    get_option(&argptr, &bit) &&
>> -	    bit >= 0 &&
>> -	    bit < NCAPINTS * 32)
>> -		setup_clear_cpu_cap(bit);
>> +				sizeof(arg))) {
>> +		/* cpuid bit numbers are mostly three digits */
>> +		enum  { nints = sizeof(arg)/(3+1) + 1 };
> 
> Not sure what the digits have to do with the stack space of an int array.
> 
> We should have enough stack to afford some more than 8.

sizeof(arg) == 32; room enough for eight three-digit with their trailing commas.   If sizeof(arg) == 1024 instead then there'd be more than enough room to remove every single feature.   TBH, 512 is more than enough for the 89 flags I have listed on this machine I'm looking at here.   I'll grow sizeof(arg) and nints accordingly.

> 
> Would be good to have a warning if the arguments are longer.
> 

Yes, I should definitely do that -- coming to a V2 soon.


> Maybe it would be simpler to fix the early arg parser
> to allow multiple instances again? That would also avoid the limit,
> and keep everything compatible.
> 

I did wonder about that.   However, cmdline_find_option() is specifically documented as 

 * Find a non-boolean option (i.e. option=argument). In accordance with
 * standard Linux practice, if this option is repeated, this returns the
 * last instance on the command line.

And since that appeared in 2017 I decided to stick with the new-fangled interface :)   This is a little-used feature; I'm not sure it's worth the effort of parsing the command line for the old style.  What do you think?

jch


> -Andi
> 
> 
>> +		int i, bits[nints];
>> +
>> +		get_options(arg, nints, bits);
>> +		for (i = 1; i <= bits[0]; i++) {
>> +			if (bits[i] >= 0 && bits[i] < NCAPINTS * 32)
>> +				setup_clear_cpu_cap(bits[i]);
>> +		}
>> +	}
>> }
>> 
>> /*
>> -- 
>> 2.25.3
>> 

Re: [PATCH 1/1] x86/fpu: Allow clearcpuid= to clear several bits

[Andi Kleen]

> 
> I did wonder about that.   However, cmdline_find_option() is specifically documented as 
> 
>  * Find a non-boolean option (i.e. option=argument). In accordance with
>  * standard Linux practice, if this option is repeated, this returns the
>  * last instance on the command line.

Okay so would need a special version that uses the first and an option
to pass the cmdline string.

> 
> And since that appeared in 2017 I decided to stick with the new-fangled interface :)   This is a little-used feature; I'm not sure it's worth the effort of parsing the command line for the old style.  What do you think?

I'm not sure it's that little used.  We use it quite a bit for testing
and workarounds, and it might be that some of those are deployed.

-Andi