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 <[email protected]> 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/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
