Re: [PATCH v6 1/3] misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] misc: fastrpc: double of_node_put() in fastrpc_cb_devices_create()
- [High] misc: fastrpc: missing DMA and IOMMU setup in fastrpc_cb_devices_create()
- [High] misc: fastrpc: premature misc_register() before fastrpc_cb_devices_create()
--

commit 17aff65cbac844472a8706f83a0e300c3444f667
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

This commit replaces of_platform_populate() and the separate
fastrpc_cb_driver platform driver with manual iteration over child DT nodes
in fastrpc_cb_devices_create() and cleanup in fastrpc_cb_devices_destroy()
so context banks are initialized synchronously during fastrpc_rpmsg_probe().

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index af18ff1992eed..caa479f304c1c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static void fastrpc_cb_remove(struct platform_device *pdev)
> +static void fastrpc_cb_devices_destroy(struct rpmsg_device *rpdev)
>  {
> -	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(pdev->dev.parent);
> -	struct fastrpc_session_ctx *sess = dev_get_drvdata(&pdev->dev);
> +	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> +	struct device *rdev = &rpdev->dev;
>  	unsigned long flags;
>  	int i;
>  
>  	spin_lock_irqsave(&cctx->lock, flags);
> -	for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
> -		if (cctx->session[i].sid == sess->sid) {
> -			cctx->session[i].valid = false;
> -			cctx->sesscount--;
> -		}
> -	}
> +	for (i = 0; i < cctx->sesscount; i++)
> +		cctx->session[i].valid = false;
>  	spin_unlock_irqrestore(&cctx->lock, flags);
> +
> +	device_for_each_child_reverse(rdev, NULL, of_platform_device_destroy);
>  }
>  
> -static const struct of_device_id fastrpc_match_table[] = {
> -	{ .compatible = "qcom,fastrpc-compute-cb" },
> -	{ }
> -};
> +static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev)
> +{
> +	struct device *rdev = &rpdev->dev;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int err;
>  
> -static struct platform_driver fastrpc_cb_driver = {
> -	.probe = fastrpc_cb_probe,
> -	.remove = fastrpc_cb_remove,
> -	.driver = {
> -		.name = "qcom,fastrpc-cb",
> -		.of_match_table = fastrpc_match_table,
> -		.suppress_bind_attrs = true,
> -	},
> -};
> +	for_each_available_child_of_node(rdev->of_node, np) {
> +		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
> +			of_node_put(np);
> +			continue;
> +		}

[Severity: High]
Won't calling of_node_put(np) before continue in fastrpc_cb_devices_create()
cause a double-put on np when an available child node is not compatible with
"qcom,fastrpc-compute-cb"?

for_each_available_child_of_node() advances by calling
of_get_next_available_child() -> of_get_next_status_child() in
drivers/of/base.c:

	of_node_put(prev);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
	return next;

Because the loop increment expression dereferences prev->sibling and
automatically calls of_node_put(prev) on each step, dropping the reference on
np explicitly before continue underflows the struct device_node reference
count and can lead to a use-after-free.

> +
> +		pdev = of_platform_device_create(np, NULL, rdev);
> +		if (!pdev) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return -ENODEV;
> +		}
> +
> +		err = fastrpc_cb_init(pdev);
> +		if (err) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
With fastrpc_cb_driver removed, how are DMA and IOMMU configuration and
probe deferral handled for the context bank platform devices created in
fastrpc_cb_devices_create()?

Previously, binding each qcom,fastrpc-compute-cb device to fastrpc_cb_driver
went through really_probe() in drivers/base/dd.c, which checked supplier
links via device_links_check_suppliers(dev), set dev->driver, and called
platform_dma_configure() in drivers/base/platform.c:

	if (is_of_node(fwnode)) {
		ret = of_dma_configure(dev, to_of_node(fwnode), true);
	} else if (is_acpi_device_node(fwnode)) {
		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
		ret = acpi_dma_configure(dev, attr);
	}
	/* @dev->driver may not be valid when we're called from the IOMMU layer */
	if (ret || !drv || to_platform_driver(drv)->driver_managed_dma)
		return ret;

	ret = iommu_device_use_default_domain(dev);

Without a bound platform_driver or an explicit of_dma_configure() call in
fastrpc_cb_devices_create() or fastrpc_cb_init(), pdev->dev.driver is always
NULL:

1. Because the iommus property resides on the child compute-cb DT nodes
   rather than the parent rpmsg node, fw_devlink only creates proxy
   sync_state links on rdev. During of_platform_device_create() ->
   device_add(), iommu_init_device() in drivers/iommu/iommu.c calls
   dev->bus->dma_configure(dev) with dev->driver == NULL and ignores its
   return value. If the SMMU has not probed yet and of_iommu_configure()
   returns -EPROBE_DEFER in of_dma_configure_id() (drivers/of/device.c), it
   returns early before arch_setup_dma_ops(dev, coherent), yet
   fastrpc_cb_init() still succeeds and marks sess->valid = true with
   unconfigured DMA ops and no IOMMU translation. A subsequent
   bus_iommu_probe() replay of of_dma_configure_id() also overwrites
   *dev->dma_mask with the 32-bit coherent_dma_mask, clobbering the 34-bit
   mask set via dma_set_mask() in fastrpc_cb_init().

2. When CONFIG_IOMMU_API is disabled, iommu_bus_notifier is not registered
   and of_dma_configure() is never called on pdev->dev at all.

3. Even when the SMMU has already probed, platform_dma_configure() returns
   before calling iommu_device_use_default_domain(dev) because dev->driver
   is NULL (leaving group->owner_cnt at 0 while fastrpc performs DMA
   allocations and dma_buf mappings on sess->dev), and managed supplier
   device links to pdev->dev remain unactivated because
   device_links_driver_bound() never runs.

> @@ -2639,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	data->rpdev = rpdev;
>  	dev_set_drvdata(&rpdev->dev, data);
>  
> -	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	err = fastrpc_cb_devices_create(rpdev);
>  	if (err)
>  		goto err_deregister_fdev;
>  

[Severity: High]
Could calling fastrpc_cb_devices_create() after fastrpc_device_register() in
fastrpc_rpmsg_probe() still expose partially initialized sessions to
userspace and trigger a use-after-free on failure?

Earlier in fastrpc_rpmsg_probe(), fastrpc_device_register() calls
misc_register() before data->refcount, data->users, data->lock, data->ctx_idr,
and data->rpdev are initialized, and before fastrpc_cb_devices_create(rpdev)
is called (as well as before ADSP duplicate session creation in "misc:
fastrpc: move ADSP duplicate session creation to the driver"). Inside
fastrpc_cb_init(), sess->valid = true and cctx->sesscount++ are also published
under cctx->lock before dma_set_mask() is executed:

	sess = &cctx->session[cctx->sesscount++];
	sess->used = false;
	sess->valid = true;
	sess->dev = dev;
	sess->sid = sid;
	...
	spin_unlock_irqrestore(&cctx->lock, flags);
	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
	if (rc) {
		dev_err(dev, "%u-bit DMA enable failed\n", dma_bits);
		return rc;
	}

Because the miscdevices are already registered while
fastrpc_cb_devices_create() iterates over the DT child nodes, a concurrent
fastrpc_device_open() -> fastrpc_session_alloc() can still observe zero or a
partial set of sessions, or acquire an earlier session and take a reference
on data via fastrpc_channel_ctx_get(cctx).

Unlike of_platform_populate() which previously returned 0 even if child
device creation failed, if dma_set_mask() fails or a later CB node fails in
of_platform_device_create() or fastrpc_cb_init(), fastrpc_cb_devices_create()
destroys the CB platform devices and fastrpc_rpmsg_probe() jumps to
err_deregister_fdev -> err_free_data: kfree(data), freeing data while an open
file descriptor still holds references to fl->cctx and fl->sctx->dev. Should
fastrpc_device_register() be called only after cctx and all CB sessions have
been fully initialized?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-dup-sessions-v6-0-578ab52ba66c@oss.qualcomm.com?part=1