Re: [PATCH v5 5/7] arm_mpam: Add device tree support for MSC probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] resctrl: mpam: DT MSCs all get id -1 and collide in
  mpam_vmsc_find()
- [Medium] resctrl: mpam: stale error returned by
  mpam_dt_parse_resources()
- [Medium] resctrl: mpam: uncleared cpumask in
  mpam_get_cpumask_from_cache_id()
- [Medium] arm_mpam: undocumented DT bindings and broken child handling
  in MSC probe
- [Medium] arm_mpam: broken DT parent and error checks in
  update_msc_accessibility()
--

commit 6c78e8406d4a4b369efac7e14139c25fdb9faa61
Author: James Morse <james.morse@arm.com>

arm_mpam: Add device tree support for MSC probing

This commit adds Device Tree probing support for MPAM MSCs, parsing MSC
and cache nodes to determine cache IDs, CPU accessibility masks, and RIS
entries.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index bea231dac7e66..78caa8ae13490 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
[ ... ]
> @@ -263,6 +266,171 @@ static void mpam_free_garbage(void)
[ ... ]
> +/*
> + * The cacheinfo structures are only populated when CPUs are online.
> + * This helper walks the device tree to include offline CPUs too.
> + */
> +int mpam_get_cpumask_from_cache_id(u32 cache_id, u32 cache_level,
> +				   cpumask_t *affinity)
> +{
> +	int cpu;
> +
> +	if (!acpi_disabled)
> +		return acpi_pptt_get_cpumask_from_cache_id(cache_id, affinity);
> +
> +	for_each_possible_cpu(cpu) {
> +		struct device_node *cpu_node __free(device_node) = of_get_cpu_node(cpu, NULL);
> +		if (!cpu_node) {
> +			pr_err("Failed to find cpu%d device node\n", cpu);
> +			return -ENOENT;
> +		}
> +
> +		__mpam_get_cpumask_from_cache_id(cpu, cpu_node, cache_id,
> +						 cache_level, affinity);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does the Device Tree path in mpam_get_cpumask_from_cache_id() leave stale or
uninitialized bits in affinity by not calling cpumask_clear(affinity) before
the loop?

Unlike acpi_pptt_get_cpumask_from_cache_id(), which clears the output mask on
entry, __mpam_get_cpumask_from_cache_id() only sets matching bits with
cpumask_set_cpu(). In traffic_matches_l3(), tmp_cpumask is allocated with
alloc_cpumask_var() without __GFP_ZERO when CONFIG_CPUMASK_OFFSTACK=y and
passed via find_l3_equivalent_bitmask() to mpam_get_cpumask_from_cache_id(),
leaving uninitialized heap bits in the cpumask_equal() comparison.

[ ... ]

> +static int mpam_dt_parse_resource(struct mpam_msc *msc, struct device_node *np,
> +				  u8 ris_idx)
> +{
> +	int err = 0;
> +	u32 level = 0;
> +	u32 cache_id;
> +	struct device *dev = &msc->pdev->dev;
> +	struct device_node *cache __free(device_node) = NULL;
> +	struct device_node *parent __free(device_node) = of_get_parent(np);
> +
> +	if (of_device_is_compatible(np, "arm,mpam-cache")) {
> +		cache = of_parse_phandle(np, "arm,mpam-device", 0);
> +		if (!cache) {
> +			dev_err_once(dev, "Failed to read phandle\n");
> +			return -EINVAL;
> +		}
> +	} else if (of_device_is_compatible(parent, "cache")) {
> +		cache = of_node_get(parent);
> +	} else {
> +		/* For now, only caches are supported */
> +		cache = NULL;
> +		return err;
> +	}
> +
> +	err = of_property_read_u32(cache, "cache-level", &level);
> +	if (err) {
> +		dev_err_once(dev, "Failed to read cache-level\n");
> +		return err;
> +	}
> +
> +	cache_id = cache_of_calculate_id(cache);
> +	if (cache_id == ~0U) {
> +		dev_err_once(dev, "Failed to calculate cache-id\n");
> +		return -ENOENT;
> +	}
> +
> +	return mpam_ris_create(msc, ris_idx, MPAM_CLASS_CACHE, level, cache_id);
> +}

[Severity: Medium]
Does mpam_dt_parse_resource() check the wrong parent node and rely on
undocumented Device Tree properties when called for a child RIS node?

Commit "dt-bindings: arm: Add MPAM MSC binding" only defines compatible, reg,
and arm,not-ready-us on the MSC node with additionalProperties: false,
without child RIS nodes, arm,mpam-cache, or arm,mpam-device. Even if a child
RIS node is passed as np from mpam_dt_parse_resources(), of_get_parent(np)
returns the arm,mpam-msc node rather than the parent cache node, so
of_device_is_compatible(parent, "cache") is false and the RIS is skipped.

> +
> +static int mpam_dt_parse_resources(struct mpam_msc *msc, void *ignored)
> +{
> +	u64 ris_idx = 0;
> +	int err, num_ris = 0;
> +	struct device_node *np;
> +
> +	np = msc->pdev->dev.of_node;
> +	for_each_available_child_of_node_scoped(np, iter) {
> +		err = of_property_read_reg(iter, 0, &ris_idx, NULL);
> +		if (!err) {
> +			/*
> +			 * ris_idx is read as u64 but indexes a 4-bit RIS selector
> +			 * (0..MPAM_MSC_MAX_NUM_RIS). Reject out-of-range values here,
> +			 * before it is narrowed to u8, so a large value cannot be
> +			 * truncated into a valid-looking index.
> +			 */
> +			if (ris_idx >= MPAM_MSC_MAX_NUM_RIS)
> +				return -EINVAL;
> +			num_ris++;
> +			err = mpam_dt_parse_resource(msc, iter, ris_idx);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	if (!num_ris)
> +		err = mpam_dt_parse_resource(msc, np, 0);
> +
> +	return err;
> +}

[Severity: Medium]
Can mpam_dt_parse_resources() return a stale error from
of_property_read_reg() and abort probing of an MSC with valid RIS children?

If an available child node with a valid reg property (incrementing num_ris)
is followed by an available child node without reg, of_property_read_reg()
sets err to a negative errno, the if (!num_ris) fallback is skipped because
num_ris is non-zero, and mpam_dt_parse_resources() returns that negative
errno to mpam_msc_drv_probe(), which tears down the MSC.

[ ... ]

> @@ -2392,15 +2550,34 @@ static int mpam_msc_setup_error_irq(struct mpam_msc *msc)
>   */
>  static void update_msc_accessibility(struct mpam_msc *msc)
>  {
> +	struct device *dev = &msc->pdev->dev;
> +	struct device_node *parent;
>  	u32 affinity_id;
>  	int err;
>  
> -	err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
> -				       &affinity_id);
> -	if (err)
> +	if (!acpi_disabled) {
> +		err = device_property_read_u32(&msc->pdev->dev, "cpu_affinity",
> +					       &affinity_id);
> +		if (err)
> +			cpumask_copy(&msc->accessibility, cpu_possible_mask);
> +		else
> +			acpi_pptt_get_cpus_from_container(affinity_id,
> +							  &msc->accessibility);
> +
> +		return;
> +	}
> +
> +	/* Where an MSC can be accessed from depends on the path to of_node. */
> +	parent = of_get_parent(msc->pdev->dev.of_node);
> +	if (parent == of_root) {
>  		cpumask_copy(&msc->accessibility, cpu_possible_mask);
> -	else
> -		acpi_pptt_get_cpus_from_container(affinity_id, &msc->accessibility);
> +	} else {
> +		if (of_device_is_compatible(parent, "cache"))
> +			get_cpumask_from_cache(parent, &msc->accessibility);
> +		else
> +			dev_err_once(dev, "Cannot determine accessibility of MSC.\n");
> +	}
> +	of_node_put(parent);
>  }

[Severity: Medium]
Does update_msc_accessibility() leave msc->accessibility empty for standalone
MSCs placed under a bus node such as /soc?

When an MSC node is under a simple-bus node rather than directly under
of_root or a cache node, update_msc_accessibility() logs an error and leaves
msc->accessibility empty, causing do_mpam_msc_drv_probe() to fail with
-EINVAL and preventing mpam_num_msc from reaching fw_num_msc. In addition,
the return value of get_cpumask_from_cache() is ignored, so if
mpam_get_cpumask_from_cache_id() fails on a later CPU after setting earlier
CPU bits, probing continues with a partial accessibility mask. Also, as noted
earlier, mpam_dt_parse_resources() and mpam_dt_parse_resource() still contain
broken parent and error handling for undocumented child RIS nodes.

[ ... ]

> @@ -2574,15 +2754,45 @@ static int mpam_msc_drv_probe(struct platform_device *pdev)
[ ... ]
> +static void mpam_dt_create_foundling_msc(void)
> +{
> +	for_each_compatible_node_scoped(cache, NULL, "cache") {
> +		for_each_child_of_node_scoped(cache, child) {
> +			if (!of_match_node(mpam_of_match, child))
> +				continue;
> +			if (!of_device_is_available(child))
> +				continue;
> +			if (of_node_check_flag(child, OF_POPULATED))
> +				continue;
> +			if (!of_platform_device_create(child, NULL, NULL))
> +				pr_err("Failed to create MSC device for %pOF\n", child);
> +		}
> +	}
> +}

[Severity: High]
Do all Device Tree MSCs get msc->id == -1 and collide in mpam_vmsc_find()?

Platform devices created from Device Tree (both via
of_platform_default_populate() and of_platform_device_create()) have
pdev->id set to PLATFORM_DEVID_NONE (-1), which do_mpam_msc_drv_probe()
copies into msc->id:

drivers/resctrl/mpam_devices.c:do_mpam_msc_drv_probe() {
	...
	msc->id = pdev->id;
	msc->pdev = pdev;
	...
}

drivers/resctrl/mpam_devices.c:mpam_vmsc_find() {
	...
	list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
		if (vmsc->msc->id == msc->id)
			return vmsc;
	}

	return mpam_vmsc_alloc(comp, msc);
}

When two DT MSCs create a RIS in the same mpam_component (for example,
multiple MSCs backing slices of the same cache, or undescribed RIS entries
placed into the shared MPAM_CLASS_UNKNOWN component during
mpam_msc_hw_probe()), mpam_vmsc_find() matches the first MSC's vmsc because
both have id == -1. The second MSC's RIS is then linked to the first MSC's
vmsc, so mpam_ris_hw_probe() and runtime configuration dereference
ris->vmsc->msc and access the first MSC without holding its locks, and
mpam_ris_destroy() later performs a use-after-free write in
clear_bit(ris->ris_idx, &msc->ris_idxs) if the first MSC was freed first.

Comparing vmsc->msc == msc in mpam_vmsc_find() (or assigning unique IDs to
DT MSCs) would avoid the collision.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-mpam-resctrl-dt-knp-support-v5-0-150335a79381@oss.qualcomm.com?part=5