linux-kernel - Re: [PATCH v4] x86/resctrl: Clear staged_config[] before and after it is used

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Dec 2022 09:36:59 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Yu, Fenghua" <fenghua.yu@...el.com>,
        Shawn Wang <shawnwang@...ux.alibaba.com>
CC:     "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "james.morse@....com" <james.morse@....com>,
        "jamie@...iainc.com" <jamie@...iainc.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v4] x86/resctrl: Clear staged_config[] before and after it
 is used

Hi Fenghua,

On 12/9/2022 6:18 PM, Yu, Fenghua wrote:
> Hi, Shawn,
> 
>> As a temporary storage, staged_config[] in rdt_domain should be cleared before
>> and after it is used. The stale value in staged_config[] could cause an MSR
>> access error.
> 
> Why should staged_config[] be cleared both before and after it's used?
> If it's cleared only before it's used, does it make more sense?

This is something we discussed during v2 of this fix.

> Usually temp variables are initialized before they are used and their values will
> be ignored after usage. Especially clearing stage_config[] in double for

Ideally temporary variables have usage that matches their lifetime - variable is
created, used and then freed. The staged_config[] array is different in that it
has temporary usage but its lifetime matches that of the domain. 
I agree that temporary variables should be initialized before they are used, but I
do prefer that we do not leave stale data around, especially since stale data was
the cause of the bug needing to be fixed with this patch.

> loop is pretty heavy code and an extra clearing stage_config[] after usage
> takes unnecessary longer time.

Could you please elaborate on this being heavy code? The clearing does not involve
interacting with the registers, it just clears the 3 entry array, one array per
domain. This is definitely not in a hot path since it is done when the user
initiates reconfiguration, either by writing to the schemata file or creating a new
resctrl directory. I thus do not see harm in doing the clearing after configuration
to ensure that no stale data is left around. I would like to better understand your
concern.

...

>> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e5a48f05e787..fee8ed86b31c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -78,6 +78,19 @@ void rdt_last_cmd_printf(const char *fmt, ...)
>>  	va_end(ap);
>>  }
>>
>> +void rdt_staged_configs_clear(void)
>> +{
>> +	struct rdt_resource *r;
>> +	struct rdt_domain *dom;
> 
> Please reorder the variable definitions in the reversed Christmas tree order.

Could you please provide more detail on how you would like this to look?
Since the lines have equal length they pass the initial ordering requirement
so there must be some finer grained requirement for lines of equal length?

Reinette