Re: [PATCH v2] sparc: populate sysfs cache topology information on sun4v

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] sparc: duplicate and unsorted cache leaves in mdesc_cache_walk()
- [High] sparc: unreset num_leaves in mdesc_cache_walk() causes hotplug OOB access
- [High] sparc: NULL dereference and uninitialized len in mdesc_find_subobject()
- [High] sparc: non-pointer fw_token in mdesc_cache_fill() passed to of_node_put()
- [Medium] sparc: mdesc_cache_walk() misses exec_unit and direct cpu-to-cache links
- [Medium] sparc: caching raw MDESC node indices breaks across mdesc_update()
- [Low] sparc: missing linux/cacheinfo.h include in arch/sparc/kernel/mdesc.c
- [Low] sparc: unguarded tlb_type in populate_cache_leaves() on SPARC32
--

commit 79d38e38d210e34aa15e627d6b85cc3737f2dc0b
Author: Jan Engelhardt <jengelh@inai.de>

sparc: populate sysfs cache topology information on sun4v

This commit populates generic cacheinfo structures in sysfs on sun4v systems
by recording each CPU's MDESC node offset and walking the MDESC execution-unit
and cache hierarchy from init_cache_level() and populate_cache_leaves().

> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> index 8e645ddac58e2..bfc3f422bffaa 100644
> --- a/arch/sparc/kernel/mdesc.c
> +++ b/arch/sparc/kernel/mdesc.c
> @@ -1196,6 +1196,12 @@ static void *fill_in_one_cpu(struct mdesc_handle *hp, u64 mp, int cpuid,
>  
>  	c = &cpu_data(cpuid);
>  	c->clock_tick = *cfreq;
> +	/*
> +	 * This will help more quickly finding the mdesc node again
> +	 * (otherwise we would have to iterate over all cpu nodes
> +	 * to locate any one with a given id).
> +	 */
> +	c->mdesc_node = mp;

[Severity: Medium]
Is it safe to cache the raw MDESC node index in cpu_data(cpuid).mdesc_node and
in leaf->fw_token across runtime Machine Description updates?

In arch/sparc/kernel/mdesc.c, an MDESC node handle (u64 mp) is an element
index into node_block(&hp->mdesc) of a specific struct mdesc_handle. On sun4v
LDOMs, when a runtime dynamic reconfiguration occurs, md_update_data() in
arch/sparc/kernel/ds.c calls mdesc_update() in arch/sparc/kernel/mdesc.c,
which replaces cur_mdesc with a newly allocated struct mdesc_handle whose raw
node indices can shift:

arch/sparc/kernel/mdesc.c:mdesc_update() {
	...
	spin_lock_irqsave(&mdesc_lock, flags);
	orig_hp = cur_mdesc;
	cur_mdesc = hp;
	spin_unlock_irqrestore(&mdesc_lock, flags);
	...
}

Because fill_in_one_cpu() only runs for all CPUs at boot or for newly added
CPUs in dr_cpu_configure(), mdesc_update() does not refresh
cpu_data(cpuid).mdesc_node for already-present CPUs. If an existing CPU is
later taken offline and brought back online via cacheinfo_cpu_online() ->
detect_cache_attributes() -> mdesc_cache_walk(), mdesc_cache_walk() grabs the
new cur_mdesc via mdesc_grab() and passes the stale cpu_data(cpuid).mdesc_node
into mdesc_find_subobject() -> mdesc_next_arc(), which scans node_block
starting at base + from + 1 without verifying that base + from is the target
CPU node.

Similarly, mdesc_cache_fill() stores the raw cache_node index in
leaf->fw_token, which cache_shared_cpu_map_setup() in drivers/base/cacheinfo.c
compares across online CPUs for equality. If a CPU is hotplugged via
dr_cpu_configure() -> add_cpu() or re-onlined after mdesc_update() shifts
cache node offsets, won't comparing fw_token values recorded from different
mdesc_handle generations fail to match shared caches or falsely match
unrelated caches?

>  
>  	tb = &trap_block[cpuid];
>  	get_mondo_data(hp, mp, tb);
> @@ -1243,6 +1249,136 @@ void mdesc_fill_in_cpu_data(cpumask_t *mask)
>  	smp_fill_in_sib_core_maps();
>  }
>  
> +/**
> + * At MDESC node @node, analyze forward links to find the @want_target,
> + * and, if @want_type is not %NULL, that it is of the requested type.
> + */
> +static u64 mdesc_find_subobject(struct mdesc_handle *mdh, u64 iu_node,
> +				const char *want_target, const char *want_type)
> +{
> +	u64 iter;
> +
> +	mdesc_for_each_arc(iter, mdh, iu_node, MDESC_ARC_TYPE_FWD) {
> +		u64 sub_node = mdesc_arc_target(mdh, iter);
> +		const char *sub_name = mdesc_node_name(mdh, sub_node);
> +		const char *type_list;
> +		int len;
> +
> +		if (strcmp(sub_name, want_target) != 0)
> +			continue;
> +		if (want_type == NULL)
> +			return sub_node;
> +		type_list = mdesc_get_property(mdh, sub_node, "type", &len);
> +		if (of_find_in_proplist(type_list, want_type, len))
> +			return sub_node;

[Severity: High]
Can mdesc_find_subobject() dereference a NULL pointer here when a matching
node does not have a "type" property?

In mdesc_find_subobject(), len is declared uninitialized on the stack. In
arch/sparc/kernel/mdesc.c, mdesc_get_property() only writes to *lenp when the
requested property is found; if sub_node lacks a "type" property,
mdesc_get_property() returns NULL and leaves len untouched (either
uninitialized or holding the length from a previous loop iteration).

mdesc_find_subobject() then passes type_list (which is NULL) and len directly
to of_find_in_proplist() in arch/sparc/kernel/prom_common.c:

arch/sparc/kernel/prom_common.c:of_find_in_proplist() {
	while (len > 0) {
		int l;

		if (!strcmp(list, match))
			return 1;
		...
	}
}

Whenever len > 0, strcmp(NULL, want_type) will be called and crash with a NULL
pointer dereference. In addition, mdesc_node_name(mdh, sub_node) can return
NULL if sub_node is out of bounds or not an MD_NODE, and sub_name is passed to
strcmp(sub_name, want_target) without a NULL check. Should both sub_name and
type_list be checked for NULL before use?

> +	}
> +	return -1;
> +}
> +
> +static void mdesc_cache_fill(struct mdesc_handle *mdh, u64 cache_node,
> +			     struct cacheinfo *leaf)

[Severity: Low]
Does arch/sparc/kernel/mdesc.c build with this change?

This file uses struct cacheinfo, struct cpu_cacheinfo, get_cpu_cacheinfo(),
and the CACHE_TYPE_UNIFIED, CACHE_TYPE_INST, and CACHE_TYPE_DATA constants in
mdesc_cache_fill() and mdesc_cache_record(), but linux/cacheinfo.h is only
included in arch/sparc/kernel/prom_common.c and not in
arch/sparc/kernel/mdesc.c. Should linux/cacheinfo.h be included in
arch/sparc/kernel/mdesc.c as well?

> +{
> +	int type_len;
> +	const u64 *level      = mdesc_get_property(mdh, cache_node, "level", NULL);
> +	const u64 *size       = mdesc_get_property(mdh, cache_node, "size", NULL);
> +	const u64 *line_size  = mdesc_get_property(mdh, cache_node, "line-size", NULL);
> +	const u64 *assoc      = mdesc_get_property(mdh, cache_node, "associativity", NULL);
> +	const char *type_list = mdesc_get_property(mdh, cache_node, "type", &type_len);
> +
> +	leaf->fw_token = (void *)(uintptr_t)cache_node;

[Severity: High]
Won't storing the integer cache_node offset in leaf->fw_token cause an invalid
pointer dereference in of_node_put() during CPU teardown when
CONFIG_OF_DYNAMIC is enabled?

On SPARC, CONFIG_OF is always enabled and paging_init() in
arch/sparc/mm/init_64.c unconditionally calls prom_build_devicetree() before
sun4v_mdesc_init(), which sets of_root so that of_have_populated_dt() always
returns true on sun4v.

When a CPU is taken offline (or if cacheinfo setup fails during
cacheinfo_cpu_online()), free_cache_attributes() in drivers/base/cacheinfo.c
calls cache_shared_cpu_map_remove():

drivers/base/cacheinfo.c:cache_shared_cpu_map_remove() {
	...
		if (of_have_populated_dt())
			of_node_put(this_leaf->fw_token);
	...
}

When CONFIG_OF_DYNAMIC is enabled, of_node_put() in drivers/of/dynamic.c
attempts to dereference this_leaf->fw_token as a struct device_node * via
kobject_put(&node->kobj), which will crash on the non-pointer MDESC node
index stored in leaf->fw_token.

[ ... ]

> +static int mdesc_cache_record(struct mdesc_handle *mdh, u64 cache_node,
> +			      unsigned int cpuid, unsigned int *leaf_count,
> +			      bool fill_it)
> +{
> +	struct cpu_cacheinfo *ci;
> +	const u64 *level = mdesc_get_property(mdh, cache_node, "level", NULL);
> +	u64 iter;
> +
> +	if (level == NULL)
> +		return -ENOENT;
> +	ci = get_cpu_cacheinfo(cpuid);
> +	if (fill_it) {
> +		if (*leaf_count >= ci->num_leaves)
> +			return -ELOOP;
> +		mdesc_cache_fill(mdh, cache_node,
> +				 &ci->info_list[(*leaf_count)++]);
> +	} else {
> +		++ci->num_leaves;
> +		if (ci->num_levels < *level)
> +			ci->num_levels = *level;
> +	}

[Severity: High]
When a CPU is taken offline and brought back online, does incrementing
ci->num_leaves without resetting it to zero first cause out-of-bounds heap
accesses on sibling CPUs?

In drivers/base/cacheinfo.c, when a CPU goes offline via
cacheinfo_cpu_pre_down() -> free_cache_attributes(), ci->info_list is freed
and set to NULL, but ci->num_leaves and ci->num_levels are not cleared:

drivers/base/cacheinfo.c:free_cache_attributes() {
	if (!per_cpu_cacheinfo(cpu))
		return;

	cache_shared_cpu_map_remove(cpu);

	kfree(per_cpu_cacheinfo(cpu));
	per_cpu_cacheinfo(cpu) = NULL;
}

When the CPU is brought back online via cacheinfo_cpu_online() ->
detect_cache_attributes() -> init_cache_level() -> mdesc_cache_walk(cpuid,
false), neither mdesc_cache_walk() nor mdesc_cache_record() resets
ci->num_leaves or ci->num_levels before walking the MDESC. As a result,
++ci->num_leaves adds to the previous count N, inflating ci->num_leaves to
2 * N (and higher on subsequent offline/online cycles).

detect_cache_attributes() then allocates 2 * N leaves in ci->info_list, while
populate_cache_leaves() -> mdesc_cache_walk(cpuid, true) starts with local
leaf_count = 0 and only populates the first N leaves, leaving the remaining N
leaves zero-initialized. Next, cache_shared_cpu_map_setup() iterates index up
to cache_leaves(cpu) (which is 2 * N) and indexes into every other online
CPU's info_list:

drivers/base/cacheinfo.c:cache_shared_cpu_map_setup() {
	for (index = 0; index < cache_leaves(cpu); index++) {
		...
		for_each_online_cpu(i) {
			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);

			if (i == cpu || !sib_cpu_ci->info_list)
				continue;/* skip if itself or no cacheinfo */
			sib_leaf = sib_cpu_ci->info_list + index;
			if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
				cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
				cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
			}
		}
	}
}

Because the other online CPUs only have N elements allocated in
sib_cpu_ci->info_list, won't sib_cpu_ci->info_list + index for index >= N read
past the end of the sibling's slab allocation, and corrupt heap memory via
cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map) if the out-of-bounds fw_token
happens to be NULL?

> +
> +	mdesc_for_each_arc(iter, mdh, cache_node, MDESC_ARC_TYPE_FWD) {
> +		u64 sub_node = mdesc_arc_target(mdh, iter);
> +		const char *sub_name = mdesc_node_name(mdh, sub_node);
> +		int ret;
> +
> +		if (strcmp(sub_name, "cache") != 0)
> +			continue;
> +		ret = mdesc_cache_record(mdh, sub_node, cpuid,
> +					 leaf_count, fill_it);
> +		if (ret != 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +int mdesc_cache_walk(unsigned int cpuid, bool fill_it)
> +{
> +	/*
> +	 * ifetch is where instruction caches are reported under.
> +	 * integer is where data cahes are reported under.
> +	 *
> +	 * The MDESCs of e.g. the SUNW,T5240 and ORCL,SPARC-S7-2 machines
> +	 * use "integer". Which machines are using "int"?
> +	 */
> +	static const char *const unit_kinds[] = {"ifetch", "integer", "int"};
> +	unsigned int leaf_count = 0;
> +	int ret = 0;
> +	struct mdesc_handle *mdh = mdesc_grab();
> +
> +	if (mdh == NULL)
> +		return -ENOENT;
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(unit_kinds); ++i) {
> +		u64 node = mdesc_find_subobject(mdh, cpu_data(cpuid).mdesc_node,
> +						"exec-unit", unit_kinds[i]);
> +		if (node == -1)
> +			continue;
> +		node = mdesc_find_subobject(mdh, node, "cache", NULL);
> +		if (node == -1)
> +			continue;
> +		ret = mdesc_cache_record(mdh, node, cpuid,
> +					 &leaf_count, fill_it);
> +		if (ret != 0)
> +			break;
> +	}

[Severity: High]
Doesn't this depth-first walk across unit_kinds[] record shared unified caches
multiple times per CPU and populate ci->info_list[] out of cache level order?

In the sun4v MDESC graph, the instruction cache chain (under "ifetch") and the
data cache chain (under "integer" or "int") both link forward to shared
unified higher-level cache nodes (for example, L1i -> L2 and L1d -> L2 on
UltraSPARC T2, or L1i -> L2i -> L3 and L1d -> L2d -> L3 on SPARC S7-2).

Because mdesc_cache_record() recurses depth-first through all forward "cache"
arcs for each execution unit without tracking already-visited cache_node
values for cpuid:

1. Every unified higher-level cache reachable from both the instruction and
   data execution units (or from an exec-unit whose "type" property list
   contains both "integer" and "int") is counted twice in ci->num_leaves and
   populated twice in ci->info_list[], creating duplicate sysfs cache index
   directories (e.g. index0..index5 on S7-2 with two L3 entries per CPU).

2. Leaves are recorded in depth-first branch order (such as L1i, L2i, L3, L1d,
   L2d, L3) rather than sorted by cache level (L1i, L1d, L2i, L2d, L3),
   contrary to the comment in populate_cache_leaves() in
   arch/sparc/kernel/prom_common.c noting that drivers/base/cacheinfo expects
   leaves to be sorted by level.

[Severity: Medium]
Also in mdesc_cache_walk(), what happens on sun4v machines whose MDESC uses
"exec_unit" (with an underscore) or links "cache" nodes directly from "cpu"
nodes?

Earlier in arch/sparc/kernel/mdesc.c, set_proc_ids() checks both "exec_unit"
and "exec-unit", and fill_in_one_cpu() walks both direct cpu -> cache forward
arcs and two-hop cpu -> node -> cache forward arcs:

arch/sparc/kernel/mdesc.c:set_proc_ids() {
	__set_proc_ids(hp, "exec_unit");
	__set_proc_ids(hp, "exec-unit");
}

Because mdesc_cache_walk() only looks for "exec-unit" intermediate nodes,
won't it fail to find any caches and return 0 with ci->num_leaves == 0 on
sun4v platforms that use "exec_unit" or direct cpu-to-cache arcs?

> +	mdesc_release(mdh);
> +	return ret;
> +}
> +
>  /* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is
>   * opened. Hold this reference until /dev/mdesc is closed to ensure
>   * mdesc data structure is not released underneath us. Store the
> diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
> index c9ec70888a394..f3f655606cc6b 100644
> --- a/arch/sparc/kernel/prom_common.c
> +++ b/arch/sparc/kernel/prom_common.c

[ ... ]

> +int populate_cache_leaves(unsigned int cpuid)
> +{
> +	/*
> +	 * Note that drivers/base/cacheinfo expects leaves to be sorted by
> +	 * level, then by index in [inst, data_or_unified]. Code should fill
> +	 * in leaves in that order (else, a call to qsort will be needed).
> +	 */
> +	int ret = -ENOENT;
> +	switch (tlb_type) {
> +#ifdef CONFIG_SPARC64
> +	case hypervisor:
> +		ret = mdesc_cache_walk(cpuid, true);
> +		break;
> +#endif
> +	default:
> +		break;
> +	}

[Severity: Low]
Does populate_cache_leaves() compile on 32-bit SPARC (CONFIG_SPARC32)?

arch/sparc/kernel/prom_common.c is built for both 32-bit and 64-bit SPARC via
obj-y += prom_common.o in arch/sparc/kernel/Makefile, whereas tlb_type is only
declared inside #ifdef CONFIG_SPARC64 in arch/sparc/include/asm/spitfire.h.
Unlike init_cache_level() above, switch (tlb_type) here is outside the
#ifdef CONFIG_SPARC64 guard. Should the entire switch (or if check, matching
init_cache_level()) be placed inside #ifdef CONFIG_SPARC64?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026180041.20089-1-jengelh@inai.de?part=1