Re: [PATCH v5 02/10] x86/resctrl: Protect against bad shift

[Reinette Chatre]

Hi Boris,

On 7/21/26 4:15 PM, Borislav Petkov wrote:
> On Tue, Jul 21, 2026 at 09:14:20AM -0700, Reinette Chatre wrote:
>> On 7/20/26 6:59 PM, Borislav Petkov wrote:
>>> On Tue, Jun 30, 2026 at 09:27:02PM -0700, Reinette Chatre wrote:
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>>> index bca782050198..9b9495174041 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>>> @@ -247,7 +247,11 @@ static __init bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>>>  
>>>>  	cpuid_count(0x80000020, subleaf, &eax, &ebx, &ecx, &edx);
>>>>  	hw_res->num_closid = edx + 1;
>>>> -	r->membw.max_bw = 1 << eax;
>>>> +	if (BITS_PER_TYPE(r->membw.max_bw) <= eax) {
>>>> +		pr_warn("Unable to support hardware's maximum bandwidth\n");
>>>> +		return false;
>>>> +	}
>>>> +	r->membw.max_bw = BIT(eax);
>>>
>>> Why?
>>>
>>> Why go all the effort if the field can be 64 bits?
>>>
>>> Why not make max_bw u64 and handle *all* possible values naturally?
>>>
>>
>> resctrl does not support values larger than u32.
>>
>> The value represented by max_bw provides the upper limit of control values that
>> user space can configure via the schemata file, which in turn represents the
>> control value programmed to hardware.
>>
>> u32 is expected in several places:
>> - Parsing of user input in bw_validate() is done via kstrtou32() placing result in
>>   "u32 bw". It is this value that is compared against max_bw and would thus need to
>>   change as a consequence.
>> - Parsing of user input to schemata file via parse_bw() places parsed bandwidth in
>>   "u32 bw_val".
>> - Upon successful parsing the control value is staged by resctrl fs in
>>   resctrl_staged_config::new_ctrl that is u32.
>> - On receipt of programming request from resctrl fs, the x86 architectural specific
>>   code programs the control value to rdt_hw_ctrl_domain::ctrl_val[] that are u32.
>>
>> resctrl, from fs to x86 architecture and now also MPAM, would thus require changes to
>> support these AMD systems. If I get the math right this will be systems that support 
>> 1073741823.875GB/s and up?
>>
>> If resctrl is required to support AMD hardware that can return up to 64bits here
>> then that requires more changes than what I am able to take on at this time. From
>> my side I prefer to stick to this resctrl safeguard for the scenario and leave support
>> for this hardware to AMD.
> 
> So that bandwidth limit thing is read from those L3QOS_BW_CONTROL_n MSRs which
> have
> 
> BW_LEN-1:0 as the range which is the bandwidth limit, at
> BW_LEN there's the unlimited bandwidth limit
> 
> and that BW_LEN is read from CPUID.
> 
> And that is, kinda 12, right now.
> 
> And your worry is that some day that limit BW_LEN might go over 31?

This is theoretical since the spec/hardware technically supports larger values
than what resctrl supports. This could take the form of actual hardware or some
model aimed at testing limits.

> 
> Which your math shows how absurd this is.
> 
> So what are we actually fixing here?
> 
> Something some AI hallucinated that it might be possible at some point in the
> very distant, dark future?
Not an AI hallucination but something encountered during my earlier attempts
at getting resctrl to pass with fewer issues reported by the various deterministic
tools, in this case a static checker, that I run as part of checking resctrl
health. 

I aim to highlight this caveat in the changelog with:
	Whether this could ever represent a reasonable bandwidth value is unknown
	but addressing the issue will appease static checkers.

I see how such change could be considered absurd. I am ok to drop it from the series
and just keep treating it's report locally as a false positive.

Reinette