linux-kernel - Re: [PATCH v21 14/18] x86/resctrl: Fill out rmid_read structure for smp

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: Wed, 26 Jun 2024 14:23:39 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>,
	<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v21 14/18] x86/resctrl: Fill out rmid_read structure for
 smp_call*() to read a counter

Hi Tony,

On 6/26/24 12:35 PM, Tony Luck wrote:
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index ff4e74594a19..877d898e8fd0 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -785,6 +785,7 @@ static void mbm_update(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>    	rr.first = false;
>>>    	rr.r = r;
>>>    	rr.d = d;
>>> +	rr.ci = NULL;
>>
>> This keeps using a struct rmid_read with random data from stack and initialize members based on
>> knowledge about how the called functions use this struct. Could you please add initialization to
>> all these places that use struct rmid_read with whatever is on the stack? This includes
>> mon_add_all_files() introduced in this series.
>> Something like below should do (in mon_add_all_files() - done as part of patch 10, mbm_update(),
>> and mon_add_all_files():

(I see now that I wrote mon_add_all_files() twice ... the latter was intended to
be rdtgroup_mondata_show()).

>> 	struct rmid_read rr = {0};
> 
> I'm making that change to the three places that struct rmid_read
> is defined as a local variable.
> 
> Should I also remove "useless" assignments now that the structure
> is zeroed by the compiler. I.e. in the above snip the rr.first = false;
> and rr.ci = NULL;

Indeed. Those are now unnecessary.

"useless" assignments are subjective. When considering this rr.ci assignment I
think it is insightful to consider its lack of a partner in mon_event_read().
Of course, rr.ci should not be set to NULL in mon_event_read() but I think it
highlights how issues can creep in since without proper struct initialization in
rdtgroup_mondata_show() and mon_add_all_files() rr.ci will contain garbage
in non-SNC/non-sum flows.

> 
> I suspect there are others.
> 
> Or do they serve as useful hints to human readers of the code?

You are of course welcome to keep those you find useful to readers of the
code. My goals with this suggestion was to (a) stop passing garbage in
struct rmid_read fields, (b) use struct rmid_read consistently.

Reinette