Re: [PATCH v5 2/3] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-02-13 at 10:22:31 -0800, Sohil Mehta wrote:
>On 2/13/2026 2:02 AM, Maciej Wieczor-Retman wrote:
>> On 2026-02-12 at 16:28:10 -0800, Sohil Mehta wrote:
>>> On 2/12/2026 7:35 AM, Maciej Wieczor-Retman wrote:
>>>
>>>>
>>>> +const char *x86_cap_name(unsigned int bit)
>>>> +{
>>>> +	unsigned int word = bit >> 5;
>>>> +	static char undef_buf[16];
>>>> +	const char *name = NULL;
>>>> +
>>>> +	if (likely(word < NCAPINTS))
>>>> +		name = x86_cap_flags[bit];
>>>> +	else if (likely(word < NCAPINTS + NBUGINTS))
>>>> +		name = x86_bug_flags[bit - 32 * NCAPINTS];
>>>> +
>>>> +	if (name)
>>>> +		return name;
>>>> +
>>>> +	snprintf(undef_buf, sizeof(undef_buf), "%u:%u", word, bit & 31);
>>>> +	return undef_buf;
>>>> +}
>>>> +
>>>
>>> Isn't it unsafe to return undef_buf because the pointer is local to this
>>> function even though it is marked static?
>>>
>>> For example, the consecutive calls below would overwrite the value
>>> stored at that address.
>>
>> I just rechecked and after I caused that pr_warn_once() to be executed the
>> strings are printed correctly.
>>
>
>The issue would only happen if both d->feature and d->depends don't have
>anything in x86_cap_flags[]. Was that true for your test?

Ah, my bad, you're right, my test just returned the proper name not the word:bit
part.

>
>> Looking a bit further down, the pointers of the returned const char* are two
>> distinct values. So I assume because there are two calls to x86_cap_name() in
>> the pr_warn_once() the two static undef_buf char arrays are statically allocated
>> and there is no overwriting, but please correct me if that assumption is wrong.
>>
>
>I don't think it works that way. As the variable is defined as "static",
>only a single undef_buf is allocated. So the same pointer should be
>returned after every call. I added the below dependency and it prints
>the same feature twice.
>
>"x86 CPU feature dependency check failure: CPU0 has '12:19' enabled but
>'12:19' disabled. Kernel..."
>
>diff --git a/arch/x86/kernel/cpu/cpuid-deps.c
>b/arch/x86/kernel/cpu/cpuid-deps.c
>index 1106a5476dca..51c482c18a2c 100644
>--- a/arch/x86/kernel/cpu/cpuid-deps.c
>+++ b/arch/x86/kernel/cpu/cpuid-deps.c
>@@ -93,6 +93,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>        { X86_FEATURE_FRED,                     X86_FEATURE_LKGS      },
>        { X86_FEATURE_SPEC_CTRL_SSBD,           X86_FEATURE_SPEC_CTRL },
>        { X86_FEATURE_LASS,                     X86_FEATURE_SMAP      },
>+       { X86_FEATURE_WRMSRNS,                  X86_FEATURE_FZRM},
>        {}
> };

So I'd think pr_cont() would work here best? Just tested it on your example and
it does work. Of course that quirk would have to be documented above the
function definition. Not sure if there is a better way while still keeping the
convenience of not having to pass any char pointers to the x86_cap_name():

pr_warn_once("x86 CPU feature dependency check failure: CPU%d has '%s' enabled ", smp_processor_id(),
	     x86_cap_name(d->feature));
pr_cont("but '%s' disabled. Kernel might be fine, but no guarantees.\n", x86_cap_name(d->depends));

 x86 CPU feature dependency check failure: CPU0 has '12:19'
 	enabled but '12:10' disabled. Kernel might be fine, but no guarantees.


>
>>>>  			pr_warn_once("x86 CPU feature dependency check failure: CPU%d has '%s' enabled but '%s' disabled. Kernel might be fine, but no guarantees.\n",
>>>>  				     smp_processor_id(),
>>>> -				     x86_feature_name(d->feature, feature_buf),
>>>> -				     x86_feature_name(d->depends, depends_buf));
>>>> +				     x86_cap_name(d->feature),
>>>> +				     x86_cap_name(d->depends));
>>>>  		}
>>>>  	}
>>>>  }
>>
>

-- 
Kind regards
Maciej Wieczór-Retman