[RFC PATCH] Use feature bit names in clearcpuid=

[RFC PATCH] Use feature bit names in clearcpuid=

[Borislav Petkov]

Hi,

so tglx hates this clearcpuid= interface where you have to give the
X86_FEATURE array indices in order to disable a feature bit for testing.
Below is a first attempt (lightly tested in a VM only) to accept the bit
names from /proc/cpuinfo too.

I say "too" because not all feature bits have names and we would still
have to support the numbers. Yeah, yuck.

An exemplary cmdline would then be something like:

clearcpuid=de,440,smca,succory,bmi1,3dnow ("succory" is wrong on
purpose).

and it says:

[    0.000000] Clearing CPUID bits: de 13:24 smca bmi1 3dnow

Also, I'm thinking we should taint the kernel when this option is used.

Thoughts?

---
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 59bf91c57aa8..10b65045fc37 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -32,14 +32,17 @@ enum cpuid_leafs
 	CPUID_7_EDX,
 };
 
+#define X86_CAP_FMT_BARE "%d:%d"
+#define x86_cap_flag_bare(flag) ((flag) >> 5), ((flag) & 31)
+
 #ifdef CONFIG_X86_FEATURE_NAMES
 extern const char * const x86_cap_flags[NCAPINTS*32];
 extern const char * const x86_power_flags[32];
 #define X86_CAP_FMT "%s"
 #define x86_cap_flag(flag) x86_cap_flags[flag]
 #else
-#define X86_CAP_FMT "%d:%d"
-#define x86_cap_flag(flag) ((flag) >> 5), ((flag) & 31)
+#define X86_CAP_FMT X86_CAP_FMT_BARE
+#define x86_cap_flag(flag) x86_cap_flag_bare((flag))
 #endif
 
 /*
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index f8ff895aaf7e..17be3c99b65d 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -244,8 +244,8 @@ static void __init fpu__init_system_ctx_switch(void)
 static void __init fpu__init_parse_early_param(void)
 {
 	char arg[128];
-	char *argptr = arg;
-	int arglen, res, bit;
+	char *argptr = arg, *opt;
+	int arglen, bit, taint = 0;
 
 #ifdef CONFIG_X86_32
 	if (cmdline_find_option_bool(boot_command_line, "no387"))
@@ -273,21 +273,45 @@ static void __init fpu__init_parse_early_param(void)
 		return;
 
 	pr_info("Clearing CPUID bits:");
-	do {
-		res = get_option(&argptr, &bit);
-		if (res == 0 || res == 3)
-			break;
-
-		/* If the argument was too long, the last bit may be cut off */
-		if (res == 1 && arglen >= sizeof(arg))
-			break;
-
-		if (bit >= 0 && bit < NCAPINTS * 32) {
-			pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
-			setup_clear_cpu_cap(bit);
+
+	while (argptr) {
+		int i;
+
+		opt = (strsep(&argptr, ","));
+		if (!opt)
+			continue;
+
+		if (!kstrtoint(opt, 10, &bit)) {
+			if (bit >= 0 && bit < NCAPINTS * 32) {
+				if (!x86_cap_flag(bit))
+					pr_cont(" " X86_CAP_FMT_BARE, x86_cap_flag_bare(bit));
+				else
+					pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
+
+				setup_clear_cpu_cap(bit);
+				taint++;
+				continue;
+			}
 		}
-	} while (res == 2);
+
+#ifdef CONFIG_X86_FEATURE_NAMES
+		for (i = 0; i < 32 * NCAPINTS; i++) {
+			if (!x86_cap_flags[i])
+				continue;
+
+			if (strcmp(x86_cap_flags[i], opt))
+				continue;
+
+			pr_cont(" %s", opt);
+			setup_clear_cpu_cap(i);
+			taint++;
+		}
+#endif
+	}
 	pr_cont("\n");
+
+	if (taint)
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 }
 
 /*

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Arvind Sankar]

On Sun, Sep 20, 2020 at 05:42:28PM +0200, Borislav Petkov wrote:
> Hi,
> 
> so tglx hates this clearcpuid= interface where you have to give the
> X86_FEATURE array indices in order to disable a feature bit for testing.
> Below is a first attempt (lightly tested in a VM only) to accept the bit
> names from /proc/cpuinfo too.
> 
> I say "too" because not all feature bits have names and we would still
> have to support the numbers. Yeah, yuck.
> 
> An exemplary cmdline would then be something like:
> 
> clearcpuid=de,440,smca,succory,bmi1,3dnow ("succory" is wrong on
> purpose).
> 
> and it says:
> 
> [    0.000000] Clearing CPUID bits: de 13:24 smca bmi1 3dnow
> 
> Also, I'm thinking we should taint the kernel when this option is used.
> 
> Thoughts?

I like it. Allowing 13:24 as input would be icing on the cake :)

Small comments below.

> @@ -273,21 +273,45 @@ static void __init fpu__init_parse_early_param(void)
>  		return;
>  
>  	pr_info("Clearing CPUID bits:");
> -	do {
> -		res = get_option(&argptr, &bit);
> -		if (res == 0 || res == 3)
> -			break;
> -
> -		/* If the argument was too long, the last bit may be cut off */
> -		if (res == 1 && arglen >= sizeof(arg))
> -			break;
> -
> -		if (bit >= 0 && bit < NCAPINTS * 32) {
> -			pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
> -			setup_clear_cpu_cap(bit);
> +
> +	while (argptr) {
> +		int i;
> +
> +		opt = (strsep(&argptr, ","));
> +		if (!opt)
> +			continue;

The !opt check is unnecessary: strsep() returns NULL iff argptr is NULL
on entry. The parentheses around strsep() also look odd.

> +
> +		if (!kstrtoint(opt, 10, &bit)) {

Could make bit unsigned and use kstrtouint().

> +			if (bit >= 0 && bit < NCAPINTS * 32) {
> +				if (!x86_cap_flag(bit))
> +					pr_cont(" " X86_CAP_FMT_BARE, x86_cap_flag_bare(bit));
> +				else
> +					pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
> +
> +				setup_clear_cpu_cap(bit);
> +				taint++;
> +				continue;
> +			}

Could always continue if it was a number, even if it was invalid, since
that shouldn't match a name in any case?

>  		}
> -	} while (res == 2);
> +
> +#ifdef CONFIG_X86_FEATURE_NAMES
> +		for (i = 0; i < 32 * NCAPINTS; i++) {
> +			if (!x86_cap_flags[i])
> +				continue;
> +
> +			if (strcmp(x86_cap_flags[i], opt))
> +				continue;
> +
> +			pr_cont(" %s", opt);
> +			setup_clear_cpu_cap(i);
> +			taint++;

We could break out of the loop here -- we can't have multiple bits with
the same name, right?

> +		}
> +#endif
> +	}
>  	pr_cont("\n");
> +
> +	if (taint)
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>  }
>  
>  /*
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Borislav Petkov]

On Sun, Sep 20, 2020 at 12:16:28PM -0400, Arvind Sankar wrote:
> Allowing 13:24 as input would be icing on the cake :)

Well, I'm kinda "meh" on that. Why, you ask?

Well, whether the user multiplies two integers or the kernel does it for
her/him, I'd prefer the user.

But that's not even the problem - whether the product of the word number
and the bit within that word, or the two supplied as a pair - either is
the wrong interface. It is ugly and not even close to even beginning to
be user-friendly.

However, we can't make it fully user-friendly yet because not all bits
have names. :-\

But you know what, that doesn't matter too because that clearcpuid=
thing is mainly for poking at sh*t and testing, not meant for users.
Thus the tainting...

So I guess the one who needs it, can go the minute distance and do the
multiplication. According to that argument, adding the string parsing is
not really needed too, but it is simple enough so WTH.

I've incorporated all your other comments, see below.

Thx!

---
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 59bf91c57aa8..10b65045fc37 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -32,14 +32,17 @@ enum cpuid_leafs
 	CPUID_7_EDX,
 };
 
+#define X86_CAP_FMT_BARE "%d:%d"
+#define x86_cap_flag_bare(flag) ((flag) >> 5), ((flag) & 31)
+
 #ifdef CONFIG_X86_FEATURE_NAMES
 extern const char * const x86_cap_flags[NCAPINTS*32];
 extern const char * const x86_power_flags[32];
 #define X86_CAP_FMT "%s"
 #define x86_cap_flag(flag) x86_cap_flags[flag]
 #else
-#define X86_CAP_FMT "%d:%d"
-#define x86_cap_flag(flag) ((flag) >> 5), ((flag) & 31)
+#define X86_CAP_FMT X86_CAP_FMT_BARE
+#define x86_cap_flag(flag) x86_cap_flag_bare((flag))
 #endif
 
 /*
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index f8ff895aaf7e..98c0e571561f 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -244,8 +244,8 @@ static void __init fpu__init_system_ctx_switch(void)
 static void __init fpu__init_parse_early_param(void)
 {
 	char arg[128];
-	char *argptr = arg;
-	int arglen, res, bit;
+	char *argptr = arg, *opt;
+	int arglen, taint = 0;
 
 #ifdef CONFIG_X86_32
 	if (cmdline_find_option_bool(boot_command_line, "no387"))
@@ -273,21 +273,48 @@ static void __init fpu__init_parse_early_param(void)
 		return;
 
 	pr_info("Clearing CPUID bits:");
-	do {
-		res = get_option(&argptr, &bit);
-		if (res == 0 || res == 3)
-			break;
 
-		/* If the argument was too long, the last bit may be cut off */
-		if (res == 1 && arglen >= sizeof(arg))
-			break;
+	while (argptr) {
+		unsigned int bit;
+
+		opt = strsep(&argptr, ",");
+
+		if (!kstrtouint(opt, 10, &bit)) {
+			if (bit < NCAPINTS * 32) {
+				if (!x86_cap_flag(bit))
+					pr_cont(" " X86_CAP_FMT_BARE, x86_cap_flag_bare(bit));
+				else
+					pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
+
+				setup_clear_cpu_cap(bit);
+				taint++;
+			}
+			/*
+			 * We're assuming there are no feature names with only
+			 * numbers in the name thus go to the next argument.
+			 */
+			continue;
+		}
+
+#ifdef CONFIG_X86_FEATURE_NAMES
+		for (bit = 0; bit < 32 * NCAPINTS; bit++) {
+			if (!x86_cap_flags[bit])
+				continue;
+
+			if (strcmp(x86_cap_flags[bit], opt))
+				continue;
 
-		if (bit >= 0 && bit < NCAPINTS * 32) {
-			pr_cont(" " X86_CAP_FMT, x86_cap_flag(bit));
+			pr_cont(" %s", opt);
 			setup_clear_cpu_cap(bit);
+			taint++;
+			break;
 		}
-	} while (res == 2);
+#endif
+	}
 	pr_cont("\n");
+
+	if (taint)
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 }
 
 /*


-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Arvind Sankar]

On Sun, Sep 20, 2020 at 07:29:54PM +0200, Borislav Petkov wrote:
> On Sun, Sep 20, 2020 at 12:16:28PM -0400, Arvind Sankar wrote:
> > Allowing 13:24 as input would be icing on the cake :)
> 
> Well, I'm kinda "meh" on that. Why, you ask?
> 
> Well, whether the user multiplies two integers or the kernel does it for
> her/him, I'd prefer the user.
> 
> But that's not even the problem - whether the product of the word number
> and the bit within that word, or the two supplied as a pair - either is
> the wrong interface. It is ugly and not even close to even beginning to
> be user-friendly.
> 
> However, we can't make it fully user-friendly yet because not all bits
> have names. :-\
> 
> But you know what, that doesn't matter too because that clearcpuid=
> thing is mainly for poking at sh*t and testing, not meant for users.
> Thus the tainting...
> 
> So I guess the one who needs it, can go the minute distance and do the
> multiplication. According to that argument, adding the string parsing is
> not really needed too, but it is simple enough so WTH.
> 
> I've incorporated all your other comments, see below.
> 
> Thx!
> 

Thanks. Maybe also mention in the documentation that names can now be
used.

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Borislav Petkov]

On Sun, Sep 20, 2020 at 02:35:27PM -0400, Arvind Sankar wrote:
> Thanks. Maybe also mention in the documentation that names can now be
> used.

Already did it as an incremental ontop. Will merge later:

---
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ffe864390c5a..49988e891d7f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -577,12 +577,17 @@
 			loops can be debugged more effectively on production
 			systems.
 
-	clearcpuid=BITNUM[,BITNUM...] [X86]
+	clearcpuid=X[,X...] [X86]
 			Disable CPUID feature X 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
+			numbers X. Note the Linux-specific bits are not necessarily
+			stable over kernel options, but the vendor-specific
 			ones should be.
+			X can also be a string as appearing in the flags: line
+			in /proc/cpuinfo which does not have the above
+			instability issue. However, not all features have names
+			in /proc/cpuinfo.
+			Note that using this option will taint your kernel.
 			Also note that user programs calling CPUID directly
 			or using the feature without checking anything
 			will still see it. This just prevents it from


-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Thomas Gleixner]

On Sun, Sep 20 2020 at 17:42, Borislav Petkov wrote:
> Hi,
>
> so tglx hates this clearcpuid= interface where you have to give the
> X86_FEATURE array indices in order to disable a feature bit for testing.
> Below is a first attempt (lightly tested in a VM only) to accept the bit
> names from /proc/cpuinfo too.
>
> I say "too" because not all feature bits have names and we would still
> have to support the numbers. Yeah, yuck.
>
> An exemplary cmdline would then be something like:
>
> clearcpuid=de,440,smca,succory,bmi1,3dnow ("succory" is wrong on
> purpose).
>
> and it says:
>
> [    0.000000] Clearing CPUID bits: de 13:24 smca bmi1 3dnow
>
> Also, I'm thinking we should taint the kernel when this option is used.
>
> Thoughts?

Yes, instead of making it differently horrible, can we finally remove that
nonsense which should have never been there in the first place?

Thanks,

        tglx

Re: [RFC PATCH] Use feature bit names in clearcpuid=

[Borislav Petkov]

On Tue, Sep 22, 2020 at 10:03:03AM +0200, Thomas Gleixner wrote:
> On Sun, Sep 20 2020 at 17:42, Borislav Petkov wrote:
> > Hi,
> >
> > so tglx hates this clearcpuid= interface where you have to give the
> > X86_FEATURE array indices in order to disable a feature bit for testing.
> > Below is a first attempt (lightly tested in a VM only) to accept the bit
> > names from /proc/cpuinfo too.
> >
> > I say "too" because not all feature bits have names and we would still
> > have to support the numbers. Yeah, yuck.
> >
> > An exemplary cmdline would then be something like:
> >
> > clearcpuid=de,440,smca,succory,bmi1,3dnow ("succory" is wrong on
> > purpose).
> >
> > and it says:
> >
> > [    0.000000] Clearing CPUID bits: de 13:24 smca bmi1 3dnow
> >
> > Also, I'm thinking we should taint the kernel when this option is used.
> >
> > Thoughts?
> 
> Yes, instead of making it differently horrible, can we finally remove that
> nonsense which should have never been there in the first place?

Fine with me - I don't need it.

I believe dhansen has a use-case or two, CCed.

-- 
Regards/Gruss,
    Boris.

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