linux-kernel - Re: [PATCH v9 25/31] fs/resctrl: Move allocation/free of closid_num_dirty

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] [day] [month] [year] [list]

Message-ID: <aMNMpMns47RkJZg-@agluck-desk3>
Date: Thu, 11 Sep 2025 15:26:44 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>, James Morse
	<james.morse@....com>
CC: Fenghua Yu <fenghuay@...dia.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>, Chen Yu
	<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v9 25/31] fs/resctrl: Move allocation/free of
 closid_num_dirty_rmid

On Wed, Sep 10, 2025 at 10:55:23AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> In subject:
> closid_num_dirty_rmid -> closid_num_dirty_rmid[]
> 
> On 8/29/25 12:33 PM, Tony Luck wrote:
> > closid_num_dirty_rmid[] is allocated in dom_data_init() during resctrl
> > initialization and freed by dom_data_exit() during resctrl exit giving
> > it the same life cycle as rmid_ptrs[].
> > 
> > Move closid_num_dirty_rmid[] allocaction/free out to
> > resctrl_l3_mon_resource_init() and resctrl_l3_mon_resource_exit() in
> > preparation for rmid_ptrs[] to be allocated on resctrl mount in support
> > of the new telemetry events.
> > 
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> >  fs/resctrl/monitor.c | 56 ++++++++++++++++++++++----------------------
> >  1 file changed, 28 insertions(+), 28 deletions(-)
> > 
> > diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> > index 33432a7f56da..d5b96aca5d03 100644
> > --- a/fs/resctrl/monitor.c
> > +++ b/fs/resctrl/monitor.c
> > @@ -805,36 +805,14 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
> >  static int dom_data_init(struct rdt_resource *r)
> >  {
> >  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> > -	u32 num_closid = resctrl_arch_get_num_closid(r);
> >  	struct rmid_entry *entry = NULL;
> >  	int err = 0, i;
> >  	u32 idx;
> >  
> >  	mutex_lock(&rdtgroup_mutex);
> 
> mutex is held during original allocation code below ...
> 
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > -		u32 *tmp;
> > -
> > -		/*
> > -		 * If the architecture hasn't provided a sanitised value here,
> > -		 * this may result in larger arrays than necessary. Resctrl will
> > -		 * use a smaller system wide value based on the resources in
> > -		 * use.
> > -		 */
> > -		tmp = kcalloc(num_closid, sizeof(*tmp), GFP_KERNEL);
> > -		if (!tmp) {
> > -			err = -ENOMEM;
> > -			goto out_unlock;
> > -		}
> > -
> > -		closid_num_dirty_rmid = tmp;
> > -	}

James,

While refactoring code, I missed moving the mutex_lock(&rdtgroup_mutex);
that was protecting the allocation of "closid_num_dirty_rmid[]" ... and
Reinette caught this change.

But looking at the code, I'm not at all sure what protection is needed
for the allocation/free of this array. The current calling sequence for
allocation is:

Linux late_init
    resctrl_arch_late_init()
	resctrl_init()
	    resctrl_l3_mon_resource_init
		 dom_data_init()

which doesn't appear to provide any scope for other CPUs to come in and
start using closid_num_dirty_rmid[]

The free path also seems safe too, as all resctrl functions should
be shutdown before calling:

resctrl_arch_exit()
    resctrl_exit()
	resctrl_l3_mon_resource_exit()
	    dom_data_exit()

and if they were not, holding the rdtgroup_mutex around:

	kfree(closid_num_dirty_rmid);
	closid_num_dirty_rmid = NULL;

would do nothing to prevent some still active resctrl function
from tripping over a NULL pointer.


So, unless I'm missing something, I'm planning to address Reinette's
find by documenting inmy commit message that holding rdtgroup_mutex
has always been unnecessary, so it is dropped as part of this refactor.

-Tony