Re: [PATCH v4 5/6] arm_mpam: Add device tree support for MSC probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] resctrl: mpam: DT MSCs all get id -1 and alias in mpam_vmsc_find()
- [Medium] resctrl: mpam: mpam_dt_parse_resources() returns stale error code
- [Medium] resctrl: mpam: undocumented DT bindings and broken parent check in MSC probe
--

commit 551add9668d2f1a68a508a323e1f2913890fe9b8
Author: James Morse <james.morse@arm.com>

arm_mpam: Add device tree support for MSC probing

This commit adds Device Tree support for probing MPAM MSC devices by
parsing MSC nodes, computing cache IDs and CPU affinity from cache nodes,
and creating RIS entries.

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 0f984fcbcae91..091c8ba2e9742 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c

[ ... ]

> @@ -263,6 +266,171 @@ static void mpam_free_garbage(void)
>  	}
>  }
>  

[ ... ]

> +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);
> +}
> +
> +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 code when an MSC node
has an available child node without a reg property after a valid RIS child
node?

In the loop over child nodes, err is overwritten on every iteration by
of_property_read_reg(). When a child node has a valid reg property, num_ris
is incremented and mpam_dt_parse_resource() sets err to 0 on success.

However, if a subsequent available child node does not have a reg property,
of_property_read_reg() sets err to -EINVAL and the if (!err) block is
skipped. Because num_ris is already non-zero, if (!num_ris) is not taken,
and mpam_dt_parse_resources() returns -EINVAL to mpam_msc_drv_probe(),
which then calls mpam_msc_drv_remove() and tears down the previously
created RIS entries.

> @@ -2394,15 +2552,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 the Device Tree parsing in mpam_dt_parse_resources(),
mpam_dt_parse_resource(), and update_msc_accessibility() match the
documented binding and expected DT hierarchy?

In the companion commit ("dt-bindings: arm: Add MPAM MSC binding"), RIS
child nodes, arm,mpam-cache, and arm,mpam-device were dropped from
Documentation/devicetree/bindings/arm/arm,mpam-msc.yaml, which sets
additionalProperties: false and only defines compatible, reg, and
arm,not-ready-us. Yet mpam_dt_parse_resources() and
mpam_dt_parse_resource() still parse child RIS nodes, arm,mpam-cache, and
arm,mpam-device.

Furthermore, when mpam_dt_parse_resources() passes a child node iter as np
to mpam_dt_parse_resource(), parent = of_get_parent(np) resolves to the MSC
node (arm,mpam-msc) rather than its parent cache node, so
of_device_is_compatible(parent, "cache") will evaluate to false.

In addition, update_msc_accessibility() checks whether parent == of_root or
is compatible with "cache". If a standalone MSC node is placed under a
standard bus node such as /soc rather than directly under /, parent ==
of_root is false, leaving msc->accessibility empty and causing
do_mpam_msc_drv_probe() to fail with -EINVAL.

[ ... ]

> @@ -2576,15 +2756,45 @@ static int mpam_msc_drv_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id mpam_of_match[] = {
> +	{ .compatible = "arm,mpam-msc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mpam_of_match);
> +
>  static struct platform_driver mpam_msc_driver = {
>  	.driver = {
>  		.name = "mpam_msc",
>  		.suppress_bind_attrs = true,
> +		.of_match_table = of_match_ptr(mpam_of_match),
>  	},
>  	.probe = mpam_msc_drv_probe,
>  	.remove = mpam_msc_drv_remove,
>  };
>  
> +/*
> + * MSCs that are declared by the firmware as being part of a cache may not
> + * be created automatically as platform devices, since there is no
> + * dedicated cache driver.
> + *
> + * Deal with theo MSCs here.
> + */
> +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]
Will all Device Tree MSCs receive msc->id == -1 and alias each other in
mpam_vmsc_find()?

On ACPI systems, acpi_mpam_parse_msc() allocates each platform_device with
platform_device_alloc("mpam_msc", tbl_msc->identifier), giving each MSC a
unique pdev->id.

On Device Tree systems, platform devices created via standard DT bus
population or via mpam_dt_create_foundling_msc() ->
of_platform_device_create() go through of_device_add(), which sets:

drivers/of/platform.c:of_device_add() {
	...
	ofdev->name = dev_name(&ofdev->dev);
	ofdev->id = PLATFORM_DEVID_NONE;
	...
}

When do_mpam_msc_drv_probe() initializes the MSC, it copies pdev->id
directly:

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

As a result, every DT-probed struct mpam_msc gets msc->id == -1. When two
or more MSCs belong to the same struct mpam_component (for example,
multiple MSCs under the same cache node or referencing the same cache, or
multiple MSCs with undescribed RIS indices placed into the shared
MPAM_CLASS_UNKNOWN component during mpam_msc_hw_probe() ->
mpam_get_or_create_ris()), mpam_ris_create_locked() calls mpam_vmsc_find():

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

Because vmsc->msc->id == msc->id evaluates -1 == -1, the second MSC (msc2)
reuses the first MSC's struct mpam_vmsc (vmsc1, where vmsc1->msc == msc1)
instead of allocating its own.

During mpam_msc_hw_probe(msc2), mpam_ris_hw_probe(ris2) resolves
ris->vmsc->msc to msc1 while holding msc2's locks:

drivers/resctrl/mpam_devices.c:mpam_ris_hw_probe() {
	struct mpam_msc *msc = ris->vmsc->msc;
	...
	lockdep_assert_held(&msc->probe_lock);
	lockdep_assert_held(&msc->part_sel_lock);
}

This triggers lockdep assertion failures on msc1 and performs unserialized
register accesses against msc1 instead of msc2.

Furthermore, during teardown in mpam_msc_drv_remove() or mpam_disable(),
destroying msc1 leaves vmsc1 alive because ris2 is still linked in
vmsc1->ris. Once msc1 is freed by mpam_free_garbage(), destroying msc2
calls mpam_ris_destroy(ris2), which dereferences the freed msc1 via
vmsc->msc:

drivers/resctrl/mpam_devices.c:mpam_ris_destroy() {
	struct mpam_vmsc *vmsc = ris->vmsc;
	struct mpam_msc *msc = vmsc->msc;
	...
	clear_bit(ris->ris_idx, &msc->ris_idxs);
}

causing a use-after-free write. Formatting msc->id with %u in
mpam_msc_setup_error_irq() and __mpam_irq_handler() will also log
msc:4294967295.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-mpam-resctrl-dt-knp-support-v4-0-bdda1e445cba@oss.qualcomm.com?part=5