[v9,02/24] x86/resctrl: kfree() rmid_ptrs from resctrl_exit()

  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.

Signed-off-by: James Morse <james.morse@arm.com>
Tested-by: Babu Moger <babu.moger@amd.com>
Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
Reviewed-by: Babu Moger <babu.moger@amd.com>
---
Changes since v5:
 * This patch is new

Changes since v6:
 * Removed struct rdt_resource argument, added __exit markers to match the
   only caller.
 * Adedd a whole stack of functions to maintain symmetry.

Changes since v7:
 * Moved the eventual kfree() call to be after rdtgroup_exit()

Changes since v8:
 * Removed an unused parameter from some unused code.
---
 arch/x86/kernel/cpu/resctrl/core.c     |  6 ++++++
 arch/x86/kernel/cpu/resctrl/internal.h |  1 +
 arch/x86/kernel/cpu/resctrl/monitor.c  | 15 +++++++++++++++
 3 files changed, 22 insertions(+)
  

Hi James,

On 2/13/2024 10:44 AM, 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.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Babu Moger <babu.moger@amd.com>
> Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
> Reviewed-by: Babu Moger <babu.moger@amd.com>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
  

Hi Reinette,

On 13/02/2024 23:14, Reinette Chatre wrote:
> On 2/13/2024 10:44 AM, 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.

> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>


Thanks!

James
  

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.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Babu Moger <babu.moger@amd.com>
> Tested-by: Carl Worth <carl@os.amperecomputing.com> # arm64
> Reviewed-by: Babu Moger <babu.moger@amd.com>
> ---

[...]


> +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?

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

Apart from that LGTM.
  

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
  

On Tue, Feb 20 2024 at 15:46, James Morse wrote:
> On 20/02/2024 15:27, David Hildenbrand wrote:
>> On 13.02.24 19:44, James Morse wrote:
>>> +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.

Which does not work because you can't acquire a mutex from hard
interrupt context.

Thanks,

        tglx
  

On 20/02/2024 15:54, Thomas Gleixner wrote:
> On Tue, Feb 20 2024 at 15:46, James Morse wrote:
>> On 20/02/2024 15:27, David Hildenbrand wrote:
>>> On 13.02.24 19:44, James Morse wrote:
>>>> +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.
> 
> Which does not work because you can't acquire a mutex from hard
> interrupt context.

Indeed - which is why that happens via schedule_work() [0]

My point was that its non-obvious where/when this will happen, so taking the lock and
forgetting about it is the simplest thing to do.


Thanks,

James


[0]
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_devices.c?h=mpam/snapshot/v6.7-rc2&id=7da1c7f9d9ef723f829bf44ed96e1fc4a46ef29f#n1299
  

On Tue, Feb 20 2024 at 16:01, James Morse wrote:
> On 20/02/2024 15:54, Thomas Gleixner wrote:
>>> With MPAM this code can be invoked from an error IRQ signalled by the hardware, so it
>>> could happen anytime.
>> 
>> Which does not work because you can't acquire a mutex from hard
>> interrupt context.
>
> Indeed - which is why that happens via schedule_work() [0]
>
> My point was that its non-obvious where/when this will happen, so taking the lock and
> forgetting about it is the simplest thing to do.

Makes sense.

Thanks,

        tglx