linux-kernel - Re: [PATCH v11 06/31] x86,fs/resctrl: Use struct rdt_domain

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: <aOBVUvy5p3915dKA@agluck-desk3>
Date: Fri, 3 Oct 2025 15:59:30 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.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 v11 06/31] x86,fs/resctrl: Use struct rdt_domain_hdr when
 reading counters

On Fri, Oct 03, 2025 at 08:34:01AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 9/25/25 1:03 PM, Tony Luck wrote:
> > Use a generic struct rdt_domain_hdr representing a generic domain
> > header in struct rmid_read in order to support other telemetry events'
> > domains besides an L3 one. Adjust the code interacting with it to the
> > new struct layout.
> 
> I'd propose a small amend to be more specific and not assume reader knows
> what rmid_read is used for:
> 
> 	struct rmid_read contains data passed around to read event counts. Use the
> 	generic domain header struct rdt_domain_hdr in struct rmid_read in order to
> 	support other telemetry events' domains besides an L3 one. Adjust the code
> 	interacting with it to the new struct layout.

Looks good. Thanks.

> 
> 
> > diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> > index 3ceef35208be..7b9fc5d3bdc8 100644
> > --- a/fs/resctrl/ctrlmondata.c
> > +++ b/fs/resctrl/ctrlmondata.c
> > @@ -550,13 +550,8 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> >  		    struct rdt_domain_hdr *hdr, struct rdtgroup *rdtgrp,
> >  		    cpumask_t *cpumask, int evtid, int first)
> >  {
> > -	struct rdt_mon_domain *d;
> >  	int cpu;
> >  
> > -	if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > -		return;
> > -	d = container_of(hdr, struct rdt_mon_domain, hdr);
> > -
> 
> Problematic snippet removed here ...
> 
Yup.

> >  	/* When picking a CPU from cpu_mask, ensure it can't race with cpuhp */
> >  	lockdep_assert_cpus_held();
> >  
> > @@ -566,7 +561,7 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> >  	rr->rgrp = rdtgrp;
> >  	rr->evtid = evtid;
> >  	rr->r = r;
> > -	rr->d = d;
> > +	rr->hdr = hdr;
> >  	rr->first = first;
> >  	if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
> >  	    resctrl_is_mbm_event(evtid)) {
> > diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> > index 4076336fbba6..32116361a5f6 100644
> > --- a/fs/resctrl/monitor.c
> > +++ b/fs/resctrl/monitor.c
> > @@ -159,7 +159,7 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
> >  			break;
> >  
> >  		entry = __rmid_entry(idx);
> > -		if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid,
> > +		if (resctrl_arch_rmid_read(r, &d->hdr, entry->closid, entry->rmid,
> >  					   QOS_L3_OCCUP_EVENT_ID, &val,
> >  					   arch_mon_ctx)) {
> >  			rmid_dirty = true;
> > @@ -424,8 +424,12 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
> >  	int err, ret;
> >  	u64 tval = 0;
> >  
> > +	if (!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3))
> > +		return -EINVAL;
> > +	d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> > +
> 
> ... but now the problem is moved to __mon_event_count() where rr->hdr can be NULL and the
> domain_header_is_valid() check is referencing NULL pointer when SNC is enabled?
> Am I missing something here? Does this work on SNC?

You are right. This likely breaks SNC.  I'll add a check for "!hdr" and
move this inside the "if (rr->is_mbm_cntr)" with a duplicate inside the
"if (rr->first)".  This duplication will be cleaned up with a later
patch to refactor __mon_event_count().
> 
> Reinette
> 

-Tony
>