Re: [PATCH v7 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 2 potential issue(s) to consider:
- [High] misc: fastrpc: missing SCM memory and pointer cleanup in fastrpc_rpmsg_probe()
- [High] misc: fastrpc: premature fastrpc_device_register() in fastrpc_rpmsg_probe()
--

commit 3d1d4146f1c73d49d10eb5163573a4b2ed34670c
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() so that context banks are synchronously
configured and initialized before fastrpc_rpmsg_probe() returns. It also
adds fastrpc_cb_devices_destroy() to invalidate sessions and destroy the
child platform devices on teardown.

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

[ ... ]

> +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;
>  
> +	for_each_available_child_of_node(rdev->of_node, np) {
> +		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb"))
> +			continue;
> +
> +		pdev = of_platform_device_create(np, NULL, rdev);
> +		if (!pdev) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return -ENODEV;
> +		}
> +
> +		/*
> +		 * No driver binds to the CB devices, so the driver core never
> +		 * runs dma_configure for them. Do it here so that dma-coherent,
> +		 * dma-ranges and the IOMMU link are set up before the session
> +		 * is used. A CB whose IOMMU setup failed is destroyed now,
> +		 * instead of being left for the next IOMMU registration to
> +		 * retry (and fail on), but does not fail the whole channel:
> +		 * failing the probe makes the DSP reopen the channel in a loop.
> +		 */
> +		err = of_dma_configure(&pdev->dev, np, true);
> +		if (!err && of_property_present(np, "iommus") &&
> +		    !device_iommu_mapped(&pdev->dev))
> +			err = -ENODEV;
> +		if (err) {
> +			if (err == -EPROBE_DEFER) {
> +				of_node_put(np);
> +				fastrpc_cb_devices_destroy(rpdev);
> +				return err;
> +			}
> +
> +			dev_err(rdev, "skipping %pOF: failed to configure DMA/IOMMU: %d\n",
> +				np, err);
> +			of_platform_device_destroy(&pdev->dev, NULL);
> +			continue;
> +		}
> +
> +		err = fastrpc_cb_init(pdev);
> +		if (err) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
>  
>  static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
>  				   bool is_secured, const char *domain)

[Severity: High]
As reported by Sashiko on v2 [1], fastrpc_rpmsg_probe() calls
fastrpc_device_register() -> misc_register() before initializing the
channel context fields and before calling fastrpc_cb_devices_create():

fastrpc_rpmsg_probe()
    fastrpc_device_register()
        misc_register(&fdev->miscdev);
    ...
    kref_init(&data->refcount);
    ...
    INIT_LIST_HEAD(&data->users);
    spin_lock_init(&data->lock);
    idr_init(&data->ctx_idr);
    data->domain_id = domain_id;
    data->rpdev = rpdev;
    dev_set_drvdata(&rpdev->dev, data);

    err = fastrpc_cb_devices_create(rpdev);
    if (err)
        goto err_deregister_fdev;

If userspace opens /dev/fastrpc-* immediately after misc_register(),
fastrpc_device_open() runs on an uninitialized spinlock and kref, can
dereference a null cctx->rpdev in dev_err_ratelimited(&cctx->rpdev->dev, ...),
and still sees zero or only partially populated sessions while
fastrpc_cb_devices_create() iterates over the child nodes (additionally,
fastrpc_cb_init() sets sess->valid = true under cctx->lock before checking
dma_set_mask()).

Furthermore, with this patch, if fastrpc_cb_devices_create() initializes
the first context bank and a concurrent fastrpc_device_open() acquires that
session, and then a subsequent child node returns -EPROBE_DEFER from
of_dma_configure() or fails in of_platform_device_create() or
fastrpc_cb_init(), fastrpc_cb_devices_create() calls
fastrpc_cb_devices_destroy(rpdev) and fastrpc_rpmsg_probe() jumps to
err_deregister_fdev:

err_deregister_fdev:
    if (data->fdevice)
        misc_deregister(&data->fdevice->miscdev);
    if (data->secure_fdevice)
        misc_deregister(&data->secure_fdevice->miscdev);

err_free_data:
    kfree(data);

Can this destroy the child platform device backing fl->sctx->dev and
unconditionally kfree(data) while the open file descriptor still holds
active references to fl->cctx and fl->sctx?

> @@ -2639,7 +2677,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]
Previously, of_platform_populate() only registered child devices and returned
0 even when child context bank probes deferred waiting for the SMMU. Now
fastrpc_cb_devices_create() synchronously calls of_dma_configure() and
fastrpc_cb_init() and returns -EPROBE_DEFER (or another error) to
fastrpc_rpmsg_probe(), jumping to err_deregister_fdev.

Earlier in fastrpc_rpmsg_probe(), fastrpc_init_reserved_mem() transfers
ownership of the reserved memory region for ADSP_DOMAIN_ID and SDSP_DOMAIN_ID
away from HLOS to the remote DSP VMIDs:

fastrpc_init_reserved_mem()
    if (domain_id == ADSP_DOMAIN_ID) {
        cctx->remote_heap_addr = res.start;
        cctx->remote_heap_size = resource_size(&res);
    }
    if (!cctx->vmcount)
        return 0;
    src_perms = BIT(QCOM_SCM_VMID_HLOS);
    return qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
                               cctx->vmperms, cctx->vmcount);

And before calling fastrpc_cb_devices_create(), fastrpc_rpmsg_probe() also
sets up:

    rdev->dma_mask = &data->dma_mask;
    ...
    idr_init(&data->ctx_idr);
    ...
    dev_set_drvdata(&rpdev->dev, data);

When fastrpc_cb_devices_create() fails or returns -EPROBE_DEFER,
err_deregister_fdev frees data without reassigning the reserved memory back
to QCOM_SCM_VMID_HLOS, without destroying data->ctx_idr, and without clearing
rpdev->dev.driver_data or rdev->dma_mask.

When the deferred probe is retried later, won't fastrpc_init_reserved_mem()
fail permanently in qcom_scm_assign_mem() because QCOM_SCM_VMID_HLOS no
longer owns the memory region, while also leaving dangling rdev->dma_mask and
drvdata pointers into freed memory (where an incoming rpmsg callback before
rpmsg_destroy_ept() in rpmsg_dev_probe() could dereference cctx in
fastrpc_rpmsg_callback())?

[1] https://lore.kernel.org/r/20260708041312.ACF681F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-dup-sessions-v7-0-03f06c014e78@oss.qualcomm.com?part=1