Re: [PATCH] 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:
- [Critical] sparc: unreset num_leaves in mdesc_cache_walk() causes OOB on CPU hotplug
- [High] sparc: non-pointer fw_token in mdesc_cache_fill() passed to of_node_put()
- [High] sparc: NULL dereference and uninitialized len in mdesc_find_subobject()
- [High] sparc: depth-first walk in mdesc_cache_record() duplicates and misorders leaves
- [High] sparc: mdesc: stale raw mdesc node indices cached in cpu_data and fw_token
- [Medium] sparc: mdesc_cache_walk() misses direct cache arcs and exec_unit nodes
- [Low] sparc: missing linux/cacheinfo.h include in arch/sparc/kernel/mdesc.c
- [Low] sparc: unguarded tlb_type in populate_cache_leaves() breaks build
--

commit 6af74801a44f46a9a2d5477d269ec4c8488504e6
Author: Jan Engelhardt <jengelh@inai.de>

sparc: populate sysfs cache topology information on sun4v

This commit populates generic sysfs cacheinfo structures on sun4v systems by
walking the hypervisor Machine Description (MDESC) 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: High]
Can caching raw MDESC node indices in cpu_data(cpuid).mdesc_node and
leaf->fw_token become stale across runtime Machine Description updates?

In arch/sparc/kernel/mdesc.c, node identifiers are raw array indices into
node_block(&hp->mdesc). When the hypervisor updates the Machine Description at
runtime via md_update_data() -> mdesc_update(), cur_mdesc is replaced and raw
node indices can shift, as noted in arch/sparc/kernel/vio.c:vio_vdev_node():

    /* Node numbers arevesion-specific and cannot be cached across
     * mdesc_update() calls...
     */

However, mdesc_update() does not refresh cpu_data(cpuid).mdesc_node for
existing CPUs, and dr_cpu_configure() only calls mdesc_fill_in_cpu_data() for
newly configured CPUs.

If an existing CPU is offlined and re-onlined after mdesc_update(),
cacheinfo_cpu_online() -> detect_cache_attributes() -> mdesc_cache_walk()
grabs the new cur_mdesc and passes the stale cpu_data(cpuid).mdesc_node into
mdesc_find_subobject() -> mdesc_next_arc():

    if (from == MDESC_NODE_NULL || from >= last_node)
        return MDESC_NODE_NULL;

    ep = base + from;

    ep++;
    for (; ep->tag != MD_NODE_END; ep++) {

Because mdesc_next_arc() does not check that base[from].tag == MD_NODE before
advancing ep and scanning until MD_NODE_END, a stale index can walk the wrong
node or scan past node_block if from lands on the final MD_NODE_END or
MD_LIST_END element.

Similarly, mdesc_cache_fill() stores the raw cache_node index in
leaf->fw_token. If CPUs are hot-added via dr_cpu_configure() -> add_cpu()
after mdesc_update(), newly added CPUs get leaf->fw_token values from the new
MDESC while already-online CPUs retain sib_leaf->fw_token values from the old
MDESC. When cache_shared_cpu_map_setup() calls cache_leaves_are_shared(),
could comparing those raw indices across different MDESC generations corrupt
shared_cpu_map?

>  
>  	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]
What happens in mdesc_find_subobject() if a matching sub_node does not have a
"type" property?

When mdesc_get_property(mdh, sub_node, "type", &len) returns NULL, len is left
uninitialized (or retains its value from the previous loop iteration). Passing
type_list == NULL and an uninitialized len to of_find_in_proplist() will
dereference NULL in strcmp(list, match) whenever len > 0:

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

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

Should mdesc_find_subobject() check that type_list != NULL before calling
of_find_in_proplist(), similar to mdesc_cache_fill()?

> +	}
> +	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 without including linux/cacheinfo.h?

mdesc_cache_fill() and mdesc_cache_record() dereference struct cacheinfo and
struct cpu_cacheinfo, use CACHE_TYPE_UNIFIED, CACHE_TYPE_INST, and
CACHE_TYPE_DATA, and call get_cpu_cacheinfo(), which are all defined in
linux/cacheinfo.h. The patch adds linux/cacheinfo.h to
arch/sparc/kernel/prom_common.c, but not to arch/sparc/kernel/mdesc.c or
arch/sparc/include/asm/mdesc.h, causing compilation of mdesc.c to fail on
CONFIG_SPARC64.

> +	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]
Is it safe to store a non-pointer MDESC node index in leaf->fw_token when
of_have_populated_dt() is true?

On SPARC (including sun4v), CONFIG_OF is enabled and prom_build_devicetree()
populates of_root at boot, so of_have_populated_dt() returns true.

In drivers/base/cacheinfo.c, cache_shared_cpu_map_remove() assumes that
whenever of_have_populated_dt() is true, this_leaf->fw_token is a struct
device_node pointer and passes it to of_node_put():

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

When a CPU is offlined (cacheinfo_cpu_pre_down() -> free_cache_attributes() ->
cache_shared_cpu_map_remove()) or cacheinfo setup fails and cleans up,
of_node_put() is called with the small integer cache_node cast to a pointer.
If CONFIG_OF_DYNAMIC is enabled, of_node_put() calls kobject_put(&node->kobj)
and dereferences the small integer as a pointer.

[ ... ]

> +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: Critical]
Will incrementing ci->num_leaves in place without resetting it to zero cause
out-of-bounds slab accesses across CPU hotplug?

When a CPU is offlined, free_cache_attributes() in drivers/base/cacheinfo.c
frees per_cpu_cacheinfo(cpu) (ci->info_list), but does not clear
ci->num_leaves or ci->num_levels:

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 that CPU is brought back online, detect_cache_attributes() calls
init_cache_level(cpuid) -> mdesc_cache_walk(cpuid, false) ->
mdesc_cache_record(), which adds to the existing ci->num_leaves value instead
of starting from 0.

For a CPU with N cache leaves, re-onlining it inflates ci->num_leaves to 2*N
(and by another N on each subsequent offline/online cycle).
detect_cache_attributes() then allocates ci->info_list with 2*N entries, while
populate_cache_leaves() only fills the first N entries, leaving entries N..2*N-1
zeroed.

Next, cache_shared_cpu_map_setup() loops index from 0 to cache_leaves(cpu) - 1
(which is 2*N - 1) and indexes directly into every other online sibling CPU's
info_list (which only has N entries):

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);
            }
        }
    }
}

For index >= N, sib_leaf points past the end of sib_cpu_ci->info_list, reading
out-of-bounds heap memory in cache_leaves_are_shared() and corrupting adjacent
slab memory via cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map) whenever the
out-of-bounds fw_token word happens to be NULL (matching the unpopulated
this_leaf->fw_token == NULL).

Should ci->num_leaves and ci->num_levels be reset to 0 before walking the
MDESC in init_cache_level() / mdesc_cache_walk(cpuid, false)?

> +
> +	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;
> +}

[Severity: High]
Does this recursive depth-first walk in mdesc_cache_record() duplicate shared
unified caches and populate leaves out of level order?

In the sun4v MDESC DAG, both the instruction cache chain (under "ifetch") and
the data cache chain (under "integer"/"int") have forward "cache" arcs that
converge on unified higher-level caches (such as unified L2 on T5240 or
unified L3 on S7-2).

Because mdesc_cache_walk() walks the entire forward "cache" chain from each
unit_kinds[] entry in depth-first order without tracking already-visited cache
nodes for cpuid:

1. Any unified L2 or L3 cache reachable from both the instruction and data
   cache paths (or from an exec-unit matching both "integer" and "int") is
   counted and populated multiple times in ci->info_list[] for the same CPU.

2. All higher-level caches reachable from "ifetch" (e.g. L2i and L3, or
   unified L2) are recorded before L1d under "integer", leaving ci->info_list
   unsorted by cache level despite drivers/base/cacheinfo expecting leaves to
   be ordered by level.

> +
> +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);

[Severity: Medium]
Could mdesc_cache_walk() miss caches on sun4v machines that use "exec_unit"
nodes or link "cpu" nodes directly to "cache" nodes?

Earlier in arch/sparc/kernel/mdesc.c, set_proc_ids() checks both "exec_unit"
and "exec-unit":

    static void set_proc_ids(struct mdesc_handle *hp)
    {
        __set_proc_ids(hp, "exec_unit");
        __set_proc_ids(hp, "exec-unit");
    }

And fill_in_one_cpu() checks both direct cpu -> cache forward arcs and 2-hop
cpu -> child -> cache forward arcs:

    mdesc_for_each_arc(a, hp, mp, MDESC_ARC_TYPE_FWD) {
        u64 j, t = mdesc_arc_target(hp, a);
        const char *t_name;

        t_name = mdesc_node_name(hp, t);
        if (!strcmp(t_name, "cache")) {
            fill_in_one_cache(c, hp, t);
            continue;
        }
        ...
    }

Because mdesc_cache_walk() only looks for "exec-unit" subobjects, sun4v
systems whose MDESC uses "exec_unit" or direct cpu -> cache arcs (such as
UltraSPARC T1) will not have any cache leaves populated.

[ ... ]

> 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

[ ... ]

> @@ -112,6 +114,38 @@ int of_find_in_proplist(const char *list, const char *match, int len)
>  }
>  EXPORT_SYMBOL(of_find_in_proplist);
>  
> +int init_cache_level(unsigned int cpuid)
> +{
> +#ifdef CONFIG_SPARC64
> +	if (tlb_type == hypervisor)
> +		return mdesc_cache_walk(cpuid, false);
> +#endif
> +	return -ENOENT;
> +}
> +
> +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 switch (tlb_type) in populate_cache_leaves() break the build on
CONFIG_SPARC32 or CONFIG_SPARC64 && !CONFIG_SMP?

arch/sparc/kernel/prom_common.c is built for both 32-bit and 64-bit SPARC, but
tlb_type and the hypervisor enumerator are only declared in
arch/sparc/include/asm/spitfire.h under #ifdef CONFIG_SPARC64.

In init_cache_level(), the tlb_type check is inside #ifdef CONFIG_SPARC64,
whereas in populate_cache_leaves(), switch (tlb_type) is outside the #ifdef,
causing an undeclared identifier error on CONFIG_SPARC32.

Additionally, arch/sparc/kernel/prom_common.c does not include asm/spitfire.h
directly, so on CONFIG_SPARC64 && !CONFIG_SMP where linux/smp.h does not pull
in asm/smp.h -> asm/spitfire.h, tlb_type and hypervisor are undeclared in both
functions.

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