From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
In filter_cpuid_features(), x86_cap_flags[] is accessed based on the
features from cpuid_dependent_features[]. However it's not verified
whether the string in x86_cap_flags[] is non-zero which could lead to
unwanted output. While currently it's not a problem (all the features in
the checked list have non-zero strings) it may cause a failure if an
entry is added that doesn't have a feature string defined.
There are two other places that open code a similar operation of
printing out the feature string or if it's not present, the word:bit
feature format.
Reuse x86_feature_name() as a common helper to validate the presence of
feature strings in filter_cpuid_features() as well as unify similar open
coded cases.
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v12:
- Remove Reviewed-by tags.
- Redo the patch message so it points out that the problem is not here
now but may happen.
- Rename
- X86_CAP_BUF_SIZE -> X86_NAMELSS_FEAT_BUFLEN
- x86_cap_name() -> x86_feature_name()
- Move exporting the header to arch/x86/kernel/cpu/cpu.h
- Remove x86_bug_flags[] handling from x86_feature_name() since it
doesn't interact with the array anywhere.
Changelog v11:
- Remove one extra tab after X86_CAP_BUF_SIZE.
- Add Reviewed-by tags from Sohil and Pawan.
Changelog v10:
- Reword the patch message a bit.
- Move x86_cap_name() declaration and X86_CAP_BUF_SIZE define to
asm/cpufeature.h.
- Don't include asm/cpu.h in cpuid-deps.c
- Extend the comment above x86_cap_name to include information on buffer
size that needs to be prepared before calling the function.
- Remove the likely(), unlikely() calls since this is not a hot path.
Changelog v9:
- 16 -> X86_CAP_BUF_SIZE.
- Add comment to the x86_cap_name().
Changelog v8:
- Move x86_cap_name() declaration from linux/cpu.h to the arch/cpu.h.
Include arch/cpu.h in the cpuid-deps.c file instead of linux/cpu.h.
Changelog v7:
- sizeof(buf) -> 16
- Rebase onto 7.01-rc1.
Changelog v6:
- Remove parts of the patch message that are redundant and just copy
what's visible in the diff.
- Redo the helper to use an external char buffer instead of a local
static string.
arch/x86/kernel/cpu/common.c | 30 +++++++++++++++++++++++++-----
arch/x86/kernel/cpu/cpu.h | 4 ++++
arch/x86/kernel/cpu/cpuid-deps.c | 19 +++----------------
3 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 76339e988304..7cfd124b3fbf 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -678,6 +678,7 @@ cpuid_dependent_features[] = {
static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
{
const struct cpuid_dependent_feature *df;
+ char feature_buf[X86_NAMELESS_FEAT_BUFLEN];
for (df = cpuid_dependent_features; df->feature; df++) {
@@ -700,7 +701,7 @@ static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
continue;
pr_warn("CPU: CPU feature %s disabled, no CPUID level 0x%x\n",
- x86_cap_flags[df->feature], df->level);
+ x86_feature_name(df->feature, feature_buf), df->level);
}
}
@@ -1637,6 +1638,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
while (arg) {
bool found __maybe_unused = false;
+ char name_buf[X86_NAMELESS_FEAT_BUFLEN];
unsigned int bit;
opt = strsep(&arg, ",");
@@ -1657,10 +1659,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
setup_clear_cpu_cap(bit);
}
/* empty-string, i.e., ""-defined feature flags */
- if (!x86_cap_flags[bit])
- pr_cont(" %d:%d\n", bit >> 5, bit & 31);
- else
- pr_cont(" %s\n", x86_cap_flags[bit]);
+ pr_cont(" %s\n", x86_feature_name(bit, name_buf));
taint++;
}
@@ -1983,6 +1982,27 @@ static void generic_identify(struct cpuinfo_x86 *c)
#endif
}
+/*
+ * Return the feature "name" if available, otherwise return the
+ * X86_FEATURE_* numerals to make it easier to identify the feature.
+ * Callers of this function need to pass a char * buffer of size
+ * X86_NAMELESS_FEAT_BUFLEN.
+ */
+const char *x86_feature_name(unsigned int bit, char *buf)
+{
+ unsigned int word = bit >> 5;
+ const char *name = NULL;
+
+ if (word < NCAPINTS)
+ name = x86_cap_flags[bit];
+
+ if (name)
+ return name;
+
+ snprintf(buf, X86_NAMELESS_FEAT_BUFLEN, "%u:%u", word, bit & 31);
+ return buf;
+}
+
/*
* This does the hard work of actually picking apart the CPU stuff...
*/
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 5c7a3a71191a..0ce29ee56442 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -91,4 +91,8 @@ static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
mode == SPECTRE_V2_EIBRS_LFENCE;
}
+#define X86_NAMELESS_FEAT_BUFLEN 16
+
+const char *x86_feature_name(unsigned int bit, char *buf);
+
#endif /* ARCH_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 146f6f8b0650..4354c0dd6d66 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -4,6 +4,8 @@
#include <linux/module.h>
#include <asm/cpufeature.h>
+#include "cpu.h"
+
struct cpuid_dep {
unsigned int feature;
unsigned int depends;
@@ -156,24 +158,9 @@ void setup_clear_cpu_cap(unsigned int feature)
do_clear_cpu_cap(NULL, feature);
}
-/*
- * Return the feature "name" if available, otherwise return
- * the X86_FEATURE_* numerals to make it easier to identify
- * the feature.
- */
-static const char *x86_feature_name(unsigned int feature, char *buf)
-{
- if (x86_cap_flags[feature])
- return x86_cap_flags[feature];
-
- snprintf(buf, 16, "%d*32+%2d", feature / 32, feature % 32);
-
- return buf;
-}
-
void check_cpufeature_deps(struct cpuinfo_x86 *c)
{
- char feature_buf[16], depends_buf[16];
+ char feature_buf[X86_NAMELESS_FEAT_BUFLEN], depends_buf[X86_NAMELESS_FEAT_BUFLEN];
const struct cpuid_dep *d;
for (d = cpuid_deps; d->feature; d++) {
--
2.53.0On Fri, Mar 27, 2026 at 03:10:52PM +0000, Maciej Wieczor-Retman wrote:
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 76339e988304..7cfd124b3fbf 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -678,6 +678,7 @@ cpuid_dependent_features[] = {
> static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
> {
> const struct cpuid_dependent_feature *df;
> + char feature_buf[X86_NAMELESS_FEAT_BUFLEN];
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;
Check your whole set pls.
...
> +/*
> + * Return the feature "name" if available, otherwise return the
> + * X86_FEATURE_* numerals to make it easier to identify the feature.
"return the X86_FEATURE word number and bit position...."
Might as well correct it to be more precise.
> + * Callers of this function need to pass a char * buffer of size
> + * X86_NAMELESS_FEAT_BUFLEN.
> + */
> +const char *x86_feature_name(unsigned int bit, char *buf)
...
> void check_cpufeature_deps(struct cpuinfo_x86 *c)
> {
> - char feature_buf[16], depends_buf[16];
> + char feature_buf[X86_NAMELESS_FEAT_BUFLEN], depends_buf[X86_NAMELESS_FEAT_BUFLEN];
Blergh, that define is too long. ;-\
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2026-03-30 at 22:00:45 +0200, Borislav Petkov wrote:
>On Fri, Mar 27, 2026 at 03:10:52PM +0000, Maciej Wieczor-Retman wrote:
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 76339e988304..7cfd124b3fbf 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -678,6 +678,7 @@ cpuid_dependent_features[] = {
>> static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
>> {
>> const struct cpuid_dependent_feature *df;
>> + char feature_buf[X86_NAMELESS_FEAT_BUFLEN];
>
>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;
>
>Check your whole set pls.
Sorry, will do.
>
>...
>
>> +/*
>> + * Return the feature "name" if available, otherwise return the
>> + * X86_FEATURE_* numerals to make it easier to identify the feature.
>
>"return the X86_FEATURE word number and bit position...."
>
>Might as well correct it to be more precise.
so maybe:
+ * Return the feature's "name" if available, otherwise return the
+ * feature's bit numerals in "word:bit" format so it's easier to identify.
?
>> + * Callers of this function need to pass a char * buffer of size
>> + * X86_NAMELESS_FEAT_BUFLEN.
>> + */
>> +const char *x86_feature_name(unsigned int bit, char *buf)
>
>...
>
>> void check_cpufeature_deps(struct cpuinfo_x86 *c)
>> {
>> - char feature_buf[16], depends_buf[16];
>> + char feature_buf[X86_NAMELESS_FEAT_BUFLEN], depends_buf[X86_NAMELESS_FEAT_BUFLEN];
>
>Blergh, that define is too long. ;-\
X86_NAMELESS_FEAT_BUF?
X86_NUM_FEAT_BUF?
Not sure how to make it much more shorter without making it also unreadable.
--
Kind regards
Maciej Wieczór-Retman
On Mon, Mar 30, 2026 at 08:42:11PM +0000, Maciej Wieczor-Retman wrote:
> + * Return the feature's "name" if available, otherwise return the
> + * feature's bit numerals in "word:bit" format so it's easier to identify.
This "numerals" thing is meh but I see why people use it.
It doesn't get any clearer than "word number and bit position".
> X86_NAMELESS_FEAT_BUF?
>
> X86_NUM_FEAT_BUF?
>
> Not sure how to make it much more shorter without making it also unreadable.
Yeah, I'm at the same spot as you - I can't think of a better name.
Since it is not too widespread, let's leave the longer, more descriptive one
and we can change it later if really needed.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On March 30, 2026 2:15:11 PM PDT, Borislav Petkov <bp@alien8.de> wrote: >On Mon, Mar 30, 2026 at 08:42:11PM +0000, Maciej Wieczor-Retman wrote: >> + * Return the feature's "name" if available, otherwise return the >> + * feature's bit numerals in "word:bit" format so it's easier to identify. > >This "numerals" thing is meh but I see why people use it. > >It doesn't get any clearer than "word number and bit position". > >> X86_NAMELESS_FEAT_BUF? >> >> X86_NUM_FEAT_BUF? >> >> Not sure how to make it much more shorter without making it also unreadable. > >Yeah, I'm at the same spot as you - I can't think of a better name. > >Since it is not too widespread, let's leave the longer, more descriptive one >and we can change it later if really needed. > >Thx. > The Right Thing™ would be to have a table in the kernel which includes *all* the feature bit names, not just the ones exported to /proc/cpuinfo, and use the unfiltered string table for kernel messages (/proc/cpuinfo being filtered via a bitmask, list, or a flag in the table.) I asked Maciej to look into it, and he has been, but I told him to not complicate this patchset too much by trying to fix everything in one shot — because it does ends up touching code in a surprisingly large set of places. It doesn't help one iota, either, that modprobe is supposed to know what the numeric bit positions mean, either. It is a pretty egregious design error.
On Mon, Mar 30, 2026 at 02:44:56PM -0700, H. Peter Anvin wrote:
> The Right Thing™ would be to have a table in the kernel which includes *all*
> the feature bit names, not just the ones exported to /proc/cpuinfo, and use
> the unfiltered string table for kernel messages (/proc/cpuinfo being
> filtered via a bitmask, list, or a flag in the table.)
Meh, not convinced. Not sure it is worth the effort and bloating if we can
simply look up the feature from word and bit number.
> It doesn't help one iota, either, that modprobe is supposed to know what the
> numeric bit positions mean, either. It is a pretty egregious design error.
That was designed in ancient times, I'd presume. :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2026-03-30 15:59, Borislav Petkov wrote: > On Mon, Mar 30, 2026 at 02:44:56PM -0700, H. Peter Anvin wrote: >> The Right Thing™ would be to have a table in the kernel which includes *all* >> the feature bit names, not just the ones exported to /proc/cpuinfo, and use >> the unfiltered string table for kernel messages (/proc/cpuinfo being >> filtered via a bitmask, list, or a flag in the table.) > > Meh, not convinced. Not sure it is worth the effort and bloating if we can > simply look up the feature from word and bit number. > Bloating? Drop in the bucket compared to other kernel messages. Getting kernel messages in cleartext is a good thing. It's testing a flag... not exactly complicated, just didn't want Maciej to complicate this one patchset with it. -hpa
On Mon, Mar 30, 2026 at 04:42:04PM -0700, H. Peter Anvin wrote:
> Bloating? Drop in the bucket compared to other kernel messages. Getting kernel
> messages in cleartext is a good thing.
>
> It's testing a flag... not exactly complicated, just didn't want Maciej to
> complicate this one patchset with it.
I mean how much bigger does
arch/x86/kernel/cpu/capflags.c
get and, as a result, bzImage?
And what for if there's a way already which works?
I'm sure you know we have bigger fish to fry. So how about we concentrate on
Ahmed's set instead of poking on this?
Once that is reviewed and massaged and discussed and applied and tested and
we're all happy, we can spend more energy on this if still and really needed.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2026-03-30 16:42, H. Peter Anvin wrote: > On 2026-03-30 15:59, Borislav Petkov wrote: >> On Mon, Mar 30, 2026 at 02:44:56PM -0700, H. Peter Anvin wrote: >>> The Right Thing™ would be to have a table in the kernel which includes *all* >>> the feature bit names, not just the ones exported to /proc/cpuinfo, and use >>> the unfiltered string table for kernel messages (/proc/cpuinfo being >>> filtered via a bitmask, list, or a flag in the table.) >> >> Meh, not convinced. Not sure it is worth the effort and bloating if we can >> simply look up the feature from word and bit number. >> > > Bloating? Drop in the bucket compared to other kernel messages. Getting kernel > messages in cleartext is a good thing. > > It's testing a flag... not exactly complicated, just didn't want Maciej to > complicate this one patchset with it. > The main issue is that since we're not allowed to use *sane* scripting languages in the kernel tree, we have to do everything in the POSIX subset of awk -- or in C. Anyway, here is a hacky and mostly untested port of mkcapflags.sh to awk which also produces an export bitmask. Another alternative would be to simply put a prefix character like + or - which can simply be skipped for kernel messages but tested for /proc/cpuinfo generation. Definitely simpler in that there is no need to haul two pointers around. -hpa
On 3/27/2026 8:10 AM, Maciej Wieczor-Retman wrote:
> arch/x86/kernel/cpu/common.c | 30 +++++++++++++++++++++++++-----
> arch/x86/kernel/cpu/cpu.h | 4 ++++
> arch/x86/kernel/cpu/cpuid-deps.c | 19 +++----------------
> 3 files changed, 32 insertions(+), 21 deletions(-)
>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
A couple of minor points below:
> @@ -1637,6 +1638,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
>
> while (arg) {
> bool found __maybe_unused = false;
> + char name_buf[X86_NAMELESS_FEAT_BUFLEN];
> unsigned int bit;
>
> opt = strsep(&arg, ",");
> @@ -1657,10 +1659,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
> setup_clear_cpu_cap(bit);
> }
> /* empty-string, i.e., ""-defined feature flags */
Can you delete this comment as well. It doesn't make sense now.
> - if (!x86_cap_flags[bit])
> - pr_cont(" %d:%d\n", bit >> 5, bit & 31);
> - else
> - pr_cont(" %s\n", x86_cap_flags[bit]);
> + pr_cont(" %s\n", x86_feature_name(bit, name_buf));
All the other equivalent buffers are defined as feature_buf, but this
one is named as name_buf. Just curious, any specific reason?
>
> taint++;
> }
On 2026-03-27 at 10:50:29 -0700, Sohil Mehta wrote:
>On 3/27/2026 8:10 AM, Maciej Wieczor-Retman wrote:
>
>> arch/x86/kernel/cpu/common.c | 30 +++++++++++++++++++++++++-----
>> arch/x86/kernel/cpu/cpu.h | 4 ++++
>> arch/x86/kernel/cpu/cpuid-deps.c | 19 +++----------------
>> 3 files changed, 32 insertions(+), 21 deletions(-)
>>
>
>Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
>
>A couple of minor points below:
>
>
>> @@ -1637,6 +1638,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
>>
>> while (arg) {
>> bool found __maybe_unused = false;
>> + char name_buf[X86_NAMELESS_FEAT_BUFLEN];
>> unsigned int bit;
>>
>> opt = strsep(&arg, ",");
>> @@ -1657,10 +1659,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
>> setup_clear_cpu_cap(bit);
>> }
>> /* empty-string, i.e., ""-defined feature flags */
>
>Can you delete this comment as well. It doesn't make sense now.
Right, will do, thanks!
>
>> - if (!x86_cap_flags[bit])
>> - pr_cont(" %d:%d\n", bit >> 5, bit & 31);
>> - else
>> - pr_cont(" %s\n", x86_cap_flags[bit]);
>> + pr_cont(" %s\n", x86_feature_name(bit, name_buf));
>
>All the other equivalent buffers are defined as feature_buf, but this
>one is named as name_buf. Just curious, any specific reason?
I think I was experimenting with cleaning up a further part of
parse_set_clear_cpuid() that printed a string from either x86_bug_flags or
x86_cap_flags. It looked ugly so I scrapped it but I guess I forgot to change
the name_buf to feature_buf. I'll correct that, thanks for noticing it :)
>>
>> taint++;
>> }
--
Kind regards
Maciej Wieczór-Retman
© 2016 - 2026 Red Hat, Inc.