Received-SPF: pass (google.com: domain of linux-kernel+bounces-73271-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <59ebda21-6164-4dff-9ba8-956d5a715048@arm.com>
Date: Tue, 20 Feb 2024 15:46:11 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 02/24] x86/resctrl: kfree() rmid_ptrs from
 resctrl_exit()
Content-Language: en-GB
To: David Hildenbrand <david@redhat.com>, x86@kernel.org,
 linux-kernel@vger.kernel.org
Cc: Fenghua Yu <fenghua.yu@intel.com>,
 Reinette Chatre <reinette.chatre@intel.com>,
 Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>,
 Babu Moger <Babu.Moger@amd.com>, shameerali.kolothum.thodi@huawei.com,
 D Scott Phillips OS <scott@os.amperecomputing.com>,
 carl@os.amperecomputing.com, lcherian@marvell.com,
 bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com,
 baolin.wang@linux.alibaba.com, Jamie Iles <quic_jiles@quicinc.com>,
 Xin Hao <xhao@linux.alibaba.com>, peternewman@google.com,
 dfustini@baylibre.com, amitsinght@marvell.com
References: <20240213184438.16675-1-james.morse@arm.com>
 <20240213184438.16675-3-james.morse@arm.com>
 <52f81c45-efa7-42c7-86f4-fc1084b1d57a@redhat.com>
From: James Morse <james.morse@arm.com>
In-Reply-To: <52f81c45-efa7-42c7-86f4-fc1084b1d57a@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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