linux-kernel - Re: [PATCH v9 02/24] x86/resctrl: kfree() rmid_ptrs from resctrl

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: <59ebda21-6164-4dff-9ba8-956d5a715048@arm.com>
Date: Tue, 20 Feb 2024 15:46:11 +0000
From: James Morse <james.morse@....com>
To: David Hildenbrand <david@...hat.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Cc: Fenghua Yu <fenghua.yu@...el.com>,
 Reinette Chatre <reinette.chatre@...el.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
 Babu Moger <Babu.Moger@....com>, shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com
Subject: Re: [PATCH v9 02/24] x86/resctrl: kfree() rmid_ptrs from
 resctrl_exit()

Hi David,

On 20/02/2024 15:27, David Hildenbrand wrote:
> On 13.02.24 19:44, James Morse wrote:
>> rmid_ptrs[] is allocated from dom_data_init() but never free()d.
>>
>> While the exit text ends up in the linker script's DISCARD section,
>> the direction of travel is for resctrl to be/have loadable modules.
>>
>> Add resctrl_put_mon_l3_config() to cleanup any memory allocated
>> by rdt_get_mon_l3_config().
>>
>> There is no reason to backport this to a stable kernel.

>> +static void __exit dom_data_exit(void)
>> +{
>> +    mutex_lock(&rdtgroup_mutex);
>> +
>> +    kfree(rmid_ptrs);
>> +    rmid_ptrs = NULL;
>> +
>> +    mutex_unlock(&rdtgroup_mutex);
> 
> Just curious: is grabbing that mutex really required?
> 
> Against which race are we trying to protect ourselves?

Not a race, but its to protect against having to think about memory ordering!


> I suspect this mutex is not required here: if we could racing with someone else, likely
> freeing that memory would not be safe either.

All the accesses to that variable take the mutex, its necessary to take the mutex to
ensure the most recently stored values are seen. In this case the array values don't
matter, but rmid_ptrs is written under the mutex too.
There is almost certainly a control dependency that means the CPU calling dom_data_exit()
will see the value of rmid_ptrs from dom_data_init() - but its much simpler to check that
all accesses take the mutex.

With MPAM this code can be invoked from an error IRQ signalled by the hardware, so it
could happen anytime.


> Apart from that LGTM.

Thanks for taking a look!


Thanks,

James