linux-kernel - Re: [PATCH 1/2] sched: topology: Fix topology validation error

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: <59409890a5760e659f8e0ee1fb931ad37d2bc621.camel@linux.intel.com>
Date: Mon, 25 Aug 2025 14:09:34 -0700
From: Tim Chen <tim.c.chen@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Vinicius Costa Gomes
 <vinicius.gomes@...el.com>, Juri Lelli <juri.lelli@...hat.com>, Dietmar
 Eggemann <dietmar.eggemann@....com>, Ben Segall <bsegall@...gle.com>, Mel
 Gorman <mgorman@...e.de>,  Valentin Schneider <vschneid@...hat.com>, Tim
 Chen <tim.c.chen@...el.com>, Vincent Guittot <vincent.guittot@...aro.org>,
 Libo Chen <libo.chen@...cle.com>, Abel Wu <wuyun.abel@...edance.com>, Len
 Brown <len.brown@...el.com>,  linux-kernel@...r.kernel.org, Chen Yu
 <yu.c.chen@...el.com>, K Prateek Nayak <kprateek.nayak@....com>, "Gautham R
 . Shenoy" <gautham.shenoy@....com>,  Zhao Liu <zhao1.liu@...el.com>
Subject: Re: [PATCH 1/2] sched: topology: Fix topology validation error

On Mon, 2025-08-25 at 09:25 +0200, Peter Zijlstra wrote:
> On Fri, Aug 22, 2025 at 01:14:14PM -0700, Tim Chen wrote:
> > From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> > 
> > As sd_numa_mask() (the function behind tl->mask() for the NUMA levels
> > of the topology) depends on the value of sched_domains_curr_level,
> > it's possible to be iterating over a level while, sd_numa_mask()
> > thinks we are in another, causing the topology validation to fail (for
> > valid cases).
> > 
> > Set sched_domains_curr_level to the current topology level while
> > iterating.
> > 
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> > Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
> > ---
> >  kernel/sched/topology.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index 977e133bb8a4..9a7ac67e3d63 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -2394,6 +2394,14 @@ static bool topology_span_sane(const struct cpumask *cpu_map)
> >  	for_each_sd_topology(tl) {
> >  		int tl_common_flags = 0;
> >  
> > +#ifdef CONFIG_NUMA
> > +		/*
> > +		 * sd_numa_mask() (one of the possible values of
> > +		 * tl->mask()) depends on the current level to work
> > +		 * correctly.
> > +		 */
> 
> This is propagating that ugly hack from sd_init(), isn't it. Except its
> pretending like its sane code... And for what?

How about the following fix for the CONFIG_NUMA case?  Will this be more sane? 

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index b958fe48e020..a92457fed135 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1758,7 +1758,7 @@ static struct sched_domain_topology_level *sched_domain_topology =
 static struct sched_domain_topology_level *sched_domain_topology_saved;

 #define for_each_sd_topology(tl)                       \
-       for (tl = sched_domain_topology; tl->mask; tl++)
+       for (tl = sched_domain_topology; tl->mask; ++tl, sched_domains_curr_level = tl->numa_level)

 void __init set_sched_topology(struct sched_domain_topology_level *tl)
 {

> 
> > +		sched_domains_curr_level = tl->numa_level;
> > +#endif
> >  		if (tl->sd_flags)
> >  			tl_common_flags = (*tl->sd_flags)();
> >  
> 		if (tl_common_flags & SD_NUMA)
> 			continue;
> 
> So how does this make any difference ?
> 
> We should never get to calling tl->mask() for NUMA.
> 

True.  I think we originally was fixing the v6.16 case which
wasn't checking for the SD_NUMA flag.  Overlooked that when we ported
the fix.

That said, I think that the for_each_sd_topology() macro needs to have
sched_domains_curr_level updated to prevent future problems.

Tim