linux-kernel - Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the configuration is completed

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]

Message-ID: <bbc21b48-58b5-6356-0248-656e22d95281@intel.com>
Date:   Mon, 24 Oct 2022 09:45:14 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Shawn Wang <shawnwang@...ux.alibaba.com>, <fenghua.yu@...el.com>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
        James Morse <james.morse@....com>, <jamie@...iainc.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the
 configuration is completed

Hi Shawn,

On 10/23/2022 7:31 PM, Shawn Wang wrote:
> On 10/22/2022 2:05 AM, Reinette Chatre wrote:
> 
> ...
> 
>>> It may not be enough to just clear staged_config[] when
>>> resctrl_arch_update_domains() exits. I think the fix needs to make
>>> sure staged_config[] can be cleared where it is set.
>>>
>>> The modification of staged_config[] comes from two paths:
>>>
>>> Path 1:
>>> rdtgroup_schemata_write() {
>>>      ...
>>>      rdtgroup_parse_resource()     // set staged_config[]
>>>      ...
>>>      resctrl_arch_update_domains()     // clear staged_config[]
>>>      ...
>>> }
>>>
>>> Path 2:
>>> rdtgroup_init_alloc() {
>>>      ...
>>>      rdtgroup_init_mba()/rdtgroup_init_cat()    // set staged_config[]
>>>      ...
>>>      resctrl_arch_update_domains()        // clear staged_config[]
>>>      ...
>>> }
>>>
>>> If we clear staged_config[] in resctrl_arch_update_domains(), goto
>>> statement for error handling between setting staged_config[] and
>>> calling resctrl_arch_update_domains() will be ignored. This can still
>>> remain the stale staged_config[].
>> ah - indeed. Thank you for catching that.
>>
>>>
>>> I think maybe it is better to put the clearing work where
>>> rdtgroup_schemata_write() and rdtgroup_init_alloc() exit.
>>>
>>
>> It may be more robust to let rdtgroup_init_alloc() follow
>> how rdtgroup_schemata_write() already ensures that it is
>> working with a clean state by clearing staged_config[] before
>> placing its staged config within.
>>
> 
> I want to make sure, do you mean just ignore the stale value and
> place the clearing work before staged_config[] is used? If so, maybe
> the only thing the fix should do is to add memset() to
> rdtgroup_init_alloc().> 

No, let us not leave stale data lying around.

The idea is that the function calling resctrl_arch_update_domains() is
responsible for initializing staged_config[] correctly and completely.
To confirm, yes, the idea is to clear the staged_config[] in
rdtgroup_init_alloc() before resctrl_arch_update_domains() is called
to follow how it is currently done in rdtgroup_schemata_write().

But, as you indicate, by itself this would leave stale data lying around.

The solution that you suggested earlier, to put the clearing work where
rdtgroup_schemata_write() and rdtgroup_init_alloc() exit, is most logical.
That makes the code symmetrical in that staged_config[] is cleared
where it is initialized and no stale data is left lying around. What was
not clear to me is how this would look in the end. Were you planning to
keep the staged_config[] clearing within rdtgroup_schemata_write() but
not do so in rdtgroup_init_alloc()? rdtgroup_schemata_write() and
rdtgroup_init_alloc() has to follow the same pattern to reduce confusion.

So, to be more robust, how about:

/* Clear staged_config[] to make sure working from a clean slate */
resctrl_arch_update_domains()
/* Clear staged_config[] to not leave stale data lying around */

Reinette