Re: [RFC PATCH v3 09/17] sched/cache: Introduce rq affinity gain calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] sched/fair: unbounded affi_llcs write in get_affi_llcs()
- [High] sched/fair: wrong llc_max bound in calc_affinity_llc_score()
- [Medium] sched/fair: missing negative node check in get_affi_llcs()
- [Medium] sched/fair: missing negative rank check in get_affi_numas()
- [Medium] sched/fair: LLC-less nodes truncate NUMA list in get_affi_numas()
--

commit c22c2cf580aaca6a95248753109bc427527a2a88
Author: Jianyong Wu <wujianyong@hygon.cn>

sched/cache: Introduce rq affinity gain calculation

This commit introduces helper functions to compute NUMA and LLC affinity
gain scores for candidate source runqueues during cache-aware load
balancing.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4eb7cede9080f..eb08ae66f4650 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -11999,6 +11999,219 @@ static bool update_llc_busiest(struct lb_env *env,
>  	 */
>  	return sgs->nr_pref_dst_llc > busiest->nr_pref_dst_llc;
>  }
> +
> +/*
> + * Get preferred LLCs for which the destination precedes the source in
> + * that LLC's intra-node affinity order. For preferred LLCs outside the
> + * source/destination node, include them only when src_llc > dst_llc,
> + * following ascending LLC ID order within that node.
> + */
> +static int get_affi_llcs(int src_llc, int dst_llc,
> +			 int *affi_llcs, int llc_max)
> +{
> +	int j = 0, src_rank, dst_rank, cur_node, node, llc;
> +
> +	if (src_llc == dst_llc)
> +		return 0;
> +
> +	cur_node = llc_to_node(src_llc);
> +	if (cur_node != llc_to_node(dst_llc))
> +		return 0;

[Severity: Medium]
What happens in get_affi_llcs() when llc_to_node(src_llc) and
llc_to_node(dst_llc) both return -1 (NUMA_NO_NODE) while src_llc != dst_llc?

In kernel/sched/topology.c, llc_to_node() returns -1 if llc < 0, if
llc_to_node_map is NULL, or if llc >= map->nr_llc (for instance during
build_sched_domains() between cpu_attach_domain() attaching newly allocated
sd_llc_id values and rebuild_llc_node_map() publishing the updated map, or
when sched_domains_free_llc_id() resets sd_llc_id to -1).

If both calls return -1, cur_node != llc_to_node(dst_llc) is false, and in
the loop below node == cur_node is false for every valid node, so every LLC
across all nodes takes the remote-node else branch and is added whenever
src_llc > dst_llc. Should get_affi_llcs() return 0 when cur_node < 0?

> +
> +	for_each_node(node) {
> +		for_each_llc_of_node(llc, node) {
> +			if (llc >= llc_max)
> +				continue;
> +			if (node == cur_node) {
> +				src_rank = affi_llc_rank_of(llc, src_llc, cur_node);
> +				dst_rank = affi_llc_rank_of(llc, dst_llc, cur_node);
> +				if (src_rank < 0 || dst_rank < 0)
> +					continue;
> +				if (src_rank > dst_rank)
> +					affi_llcs[j++] = llc;
> +			} else {
> +				if (src_llc > dst_llc)
> +					affi_llcs[j++] = llc;
> +			}
> +		}
> +	}

[Severity: Critical]
Can get_affi_llcs() overflow affi_llcs[] (which points to sd->affi_ids,
allocated with llc_max entries in alloc_sd_llc()) if llc_to_node_map is
updated concurrently?

Unlike get_affi_numas(), which checks if (j >= max) break, get_affi_llcs()
only checks if (llc >= llc_max) continue and does not bound j against
llc_max. However, for_each_llc_of_node() in kernel/sched/sched.h enters a
separate RCU read-side section on each node iteration:

    #define for_each_llc_of_node(llc, node) \
        scoped_guard(rcu) \
            for (struct llc_node_iter __ln_iter = \
                     llc_node_iter_begin(node); \
                 ((llc) = llc_node_iter_next(&__ln_iter)) >= 0;)

If rebuild_llc_node_map() publishes a new llc_to_node_map during CPU hotplug
or sched_domain rebuilds where an LLC ID freed on a lower-numbered node is
reallocated to a higher-numbered node, get_affi_llcs() can observe the same
LLC ID (< llc_max) in multiple node iterations and increment j past llc_max,
writing past the end of sd->affi_ids and causing calc_affinity_llc_score() to
read out of bounds as well. Should get_affi_llcs() check j >= llc_max (or
hold a single RCU read lock across the entire node scan)?

> +
> +	return j;
> +}
> +
> +static int get_affi_numas(int src_node, int dst_node, int *affi_nodes, int *affi, int max)
> +{
> +	int j = 0, src_dist, dst_dist, node;
> +
> +	for_each_node(node) {
> +		if (j >= max)
> +			break;
> +		src_dist = node_distance(src_node, node);
> +		dst_dist = node_distance(dst_node, node);
> +		if (src_dist < 0 || dst_dist < 0)
> +			continue;
> +		if (src_dist == dst_dist) {
> +			src_dist = affi_node_rank_of(node, src_node);
> +			dst_dist = affi_node_rank_of(node, dst_node);
> +			if (src_dist > dst_dist) {
> +				affi[j] = 1;
> +				affi_nodes[j++] = node;
> +			}
> +			continue;
> +		}

[Severity: Medium]
Should get_affi_numas() verify that src_dist and dst_dist are non-negative
after calling affi_node_rank_of(), and bail out early when
src_node == dst_node?

In kernel/sched/topology.c, affi_node_rank_of() takes its own RCU lock per
call and returns -1 if affi_node_order is NULL (for example if
rebuild_node_order() fails allocation and publishes NULL) or if a node ID is
out of bounds.

If affi_node_rank_of(node, src_node) returns a valid rank (>= 0) and a
concurrent update causes affi_node_rank_of(node, dst_node) to return -1,
src_dist > dst_dist evaluates to true (even when src_node == dst_node) and
adds the node with weight 1. By comparison, get_affi_llcs() checks
if (src_rank < 0 || dst_rank < 0) continue before comparing ranks.

[ ... ]

> +static int calc_affinity_numa_score(struct sched_domain *sd, int src_cpu, int dst_cpu,
> +			int *affi_node, int *affi, int *last_node, int *num, int llc_max)
> +{
> +	int llc, src_node, dst_node, tmp_node, score = 0, numa_count, node_max;
> +
> +	src_node = cpu_to_node(src_cpu);
> +	dst_node = cpu_to_node(dst_cpu);
> +	node_max = min_t(int, nr_node_ids, llc_max);
> +	/*
> +	 * The destination stays fixed during this source-rq scan. Reuse the
> +	 * eligible node list and weights for consecutive source CPUs in the
> +	 * same node; each rq's task counts are applied below.
> +	 */
> +	if (src_node != *last_node) {
> +		*last_node = src_node;
> +		*num = get_affi_numas(src_node, dst_node, affi_node,
> +				      affi, node_max);
> +	}

[Severity: Medium]
Can CPU-less or offline NUMA nodes cause get_affi_numas() to drop active
cache-preferred nodes when nr_node_ids > llc_max?

In alloc_sd_llc(), sd->affi_ids and sd->affi_weights are sized to llc_max
(the number of LLC IDs for online CPUs), so calc_affinity_numa_score() caps
node_max at min_t(int, nr_node_ids, llc_max).

However, get_affi_numas() iterates over for_each_node(node) (all possible
nodes in node_possible_map, including CPU-less HBM/CXL memory nodes or
offline nodes that have no LLCs) and stops once j >= max. Any lower-numbered
node without LLCs takes a slot in affi_node[] while always producing
numa_count == 0 in the for_each_llc_of_node(llc, tmp_node) loop below, which
can truncate higher-numbered NUMA nodes that actually have LLCs and tasks.

[ ... ]

> +static int calc_affinity_llc_score(struct sched_domain *sd, struct sched_domain *sd_rq,
> +			int src_cpu, int dst_cpu, int *affi_llc,
> +			int *last_llc, int *num, int llc_max)
> +{
> +	int src_llc, dst_llc, score = 0;
> +
> +	if (sd->flags & (SD_NUMA | SD_SHARE_LLC))
> +		return 0;
> +
> +	src_llc = llc_id(src_cpu);
> +	dst_llc = llc_id(dst_cpu);
> +
> +	/*
> +	 * The destination stays fixed during this source-rq scan. Reuse the
> +	 * eligible LLC list for consecutive source CPUs in the same LLC;
> +	 * each rq's task counts are applied below.
> +	 */
> +	if (src_llc != *last_llc) {
> +		*last_llc = src_llc;
> +		*num = get_affi_llcs(src_llc, dst_llc, affi_llc, llc_max);
> +	}
> +
> +	for (int i = 0; i < *num; i++) {
> +		if ((unsigned int)affi_llc[i] < llc_max)
> +			score += sd_rq->llc_counts[affi_llc[i]];
> +	}

[Severity: High]
Does calc_affinity_llc_score() check the wrong llc_max bound before indexing
sd_rq->llc_counts[]?

In lb_affi_scratch(), llc_max is read from this_rq()->sd->llc_max (the
balancing CPU's bottom sched_domain), whereas sd_rq is
rcu_dereference_all(rq->sd) for the candidate source runqueue, whose
sd_rq->llc_counts array is sized by sd_rq->llc_max (and annotated with
__counted_by_ptr(llc_max) in struct sched_domain).

While calc_affinity_numa_score() checks if (llc >= sd->llc_max) against
sd_rq->llc_max, calc_affinity_llc_score() checks
if ((unsigned int)affi_llc[i] < llc_max) against the balancing CPU's llc_max.
If sd_rq->llc_max < llc_max (for example while build_sched_domains() attaches
updated domains one CPU at a time under RCU, across reused vs. rebuilt cpuset
partitions during NOHZ balance, or if alloc_sd_llc() failed on sd_rq leaving
sd_rq->llc_max == 0 and sd_rq->llc_counts == NULL), indexing
sd_rq->llc_counts[affi_llc[i]] can read out of bounds or dereference NULL.
Should this check against sd_rq->llc_max as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029022036.4002331-1-wujianyong@hygon.cn?part=9