Re: [PATCH] sched/topology: Initialize sd_span after assignment to *sd

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 3/21/26 10:08 PM, K Prateek Nayak wrote:
> Nathan reported a kernel panic on his ARM builds after commit
> 8e8e23dea43e ("sched/topology: Compute sd_weight considering cpuset
> partitions") which was root caused to the compiler zeroing out the first
> few bytes of sd->span.
> 
> During the debug [1], it was discovered that, on some configs,
> offsetof(struct sched_domain, span) at 292 was less than
> sizeof(struct sched_domain) at 296 resulting in:
> 
>    *sd = { ... }
> 
> assignment clearing out first 4 bytes of sd->span which was initialized
> before.
> 
> The official GCC specification for "Arrays of Length Zero" [2] says:
> 
>    Although the size of a zero-length array is zero, an array member of
>    this kind may increase the size of the enclosing type as a result of
>    tail padding.
> 
> which means the relative offset of the variable length array at the end
> of the sturct can indeed be less than sizeof() the struct as a result of
> tail padding thus overwriting that data of the flexible array that
> overlapped with the padding whenever the struct is initialized as whole.
> 
> Partially revert commit 8e8e23dea43e ("sched/topology: Compute sd_weight
> considering cpuset partitions") to initialize sd_span after the fixed
> memebers of sd.
> 
> Use
> 
>    cpumask_weight_and(cpu_map, tl->mask(tl, cpu))
> 
> to calculate span_weight before initializing the sd_span.
> cpumask_and_weight() is of same complexity as cpumask_and() and the
> additional overhead is negligible.
> 
> While at it, also initialize sd->span_weight in sd_init() since
> sd_weight now captures the cpu_map constraints. Fixup the
> sd->span_weight whenever sd_span is fixed up by the generic topology
> layer.
> 


This description is a bit confusing. Fixup happens naturally since
cpu_map now reflects the changes right?

Maybe mention about that removal in build_sched_domains?

> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://lore.kernel.org/all/20260320235824.GA1176840@ax162/
> Fixes: 8e8e23dea43e ("sched/topology: Compute sd_weight considering cpuset partitions")
> Link: https://lore.kernel.org/all/a8c125fd-960d-4b35-b640-95a33584eb08@amd.com/ [1]
> Link: https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html [2]
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> Nathan, can you please check if this fixes the issue you are observing -
> it at least fixed one that I'm observing ;-)
> 
> Peter, if you would like to keep revert and enhancements separate, let
> me know and I'll spin a v2.
> ---
>   kernel/sched/topology.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 43150591914b..721ed9b883b8 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1669,17 +1669,13 @@ sd_init(struct sched_domain_topology_level *tl,
>   	struct cpumask *sd_span;
>   	u64 now = sched_clock();
>   
> -	sd_span = sched_domain_span(sd);
> -	cpumask_and(sd_span, cpu_map, tl->mask(tl, cpu));
> -	sd_weight = cpumask_weight(sd_span);
> -	sd_id = cpumask_first(sd_span);
> +	sd_weight = cpumask_weight_and(cpu_map, tl->mask(tl, cpu));
>   
>   	if (tl->sd_flags)
>   		sd_flags = (*tl->sd_flags)();
>   	if (WARN_ONCE(sd_flags & ~TOPOLOGY_SD_FLAGS,
>   		      "wrong sd_flags in topology description\n"))
>   		sd_flags &= TOPOLOGY_SD_FLAGS;
> -	sd_flags |= asym_cpu_capacity_classify(sd_span, cpu_map);
>   
>   	*sd = (struct sched_domain){
>   		.min_interval		= sd_weight,
> @@ -1715,8 +1711,15 @@ sd_init(struct sched_domain_topology_level *tl,
>   		.last_decay_max_lb_cost	= jiffies,
>   		.child			= child,
>   		.name			= tl->name,
> +		.span_weight		= sd_weight,
>   	};
>   
> +	sd_span = sched_domain_span(sd);
> +	cpumask_and(sd_span, cpu_map, tl->mask(tl, cpu));
> +	sd_id = cpumask_first(sd_span);
> +
> +	sd->flags |= asym_cpu_capacity_classify(sd_span, cpu_map);
> +
>   	WARN_ONCE((sd->flags & (SD_SHARE_CPUCAPACITY | SD_ASYM_CPUCAPACITY)) ==
>   		  (SD_SHARE_CPUCAPACITY | SD_ASYM_CPUCAPACITY),
>   		  "CPU capacity asymmetry not supported on SMT\n");
> @@ -2518,6 +2521,8 @@ static struct sched_domain *build_sched_domain(struct sched_domain_topology_leve
>   			cpumask_or(sched_domain_span(sd),
>   				   sched_domain_span(sd),
>   				   sched_domain_span(child));
> +
> +			sd->span_weight = cpumask_weight(sched_domain_span(sd));
>   		}
>   
>   	}
> @@ -2697,7 +2702,6 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
>   	/* Build the groups for the domains */
>   	for_each_cpu(i, cpu_map) {
>   		for (sd = *per_cpu_ptr(d.sd, i); sd; sd = sd->parent) {
> -			sd->span_weight = cpumask_weight(sched_domain_span(sd));
>   			if (sd->flags & SD_NUMA) {
>   				if (build_overlap_sched_groups(sd, i))
>   					goto error;
> 
> base-commit: fe7171d0d5dfbe189e41db99580ebacafc3c09ce


Other than nits in changelog:
Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>


PS: b4 am -Q was quite confused which patch to pick for 0001.
may since it was a reply to the thread. Not sure. So i pulled
each patch separate and applied.