Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] misc: fastrpc: race on ADSP duplicate sessions in fastrpc_rpmsg_probe() - [High] misc: fastrpc: dangling drvdata on error paths in fastrpc_rpmsg_probe() --
commit ea9ec3521b0228bbc7931e61d1e5ec984d8cfeaf Author: Vinayak Katoch <[email protected]> misc: fastrpc: fix init ordering race in rpmsg probe This patch moves channel context initialization and fastrpc_cb_devices_create() ahead of fastrpc_device_register() in fastrpc_rpmsg_probe() so the context is initialized before device nodes are exposed to userspace. It also updates the probe error path to clean up compute-cb devices and drop the channel context reference. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f280bff2edf05..fc09cd27af4b8 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2605,6 +2619,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > data->poll_mode_supported = soc_data->poll_mode_supported || > of_machine_get_match(fastrpc_poll_supported_machines); > > + kref_init(&data->refcount); > + atomic_set(&data->ctx_seq, 0); > + > + rdev->dma_mask = &data->dma_mask; > + dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); > + INIT_LIST_HEAD(&data->users); > + INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); > + 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_free_data; > + > switch (domain_id) { > case ADSP_DOMAIN_ID: > case MDSP_DOMAIN_ID: > @@ -2613,7 +2644,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > data->unsigned_support = false; > err = fastrpc_device_register(rdev, data, secure_dsp, domain); > if (err) > - goto err_free_data; > + goto err_depopulate; > break; > case CDSP_DOMAIN_ID: > case GDSP_DOMAIN_ID: > @@ -2621,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > /* Create both device nodes so that we can allow both Signed > and Unsigned PD */ > err = fastrpc_device_register(rdev, data, true, domain); > if (err) > - goto err_free_data; > + goto err_depopulate; > > err = fastrpc_device_register(rdev, data, false, domain); > if (err) > @@ -2629,30 +2660,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > break; > default: > err = -EINVAL; > - goto err_free_data; > + goto err_depopulate; > } > > - kref_init(&data->refcount); > - atomic_set(&data->ctx_seq, 0); > - > - rdev->dma_mask = &data->dma_mask; > - dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); > - INIT_LIST_HEAD(&data->users); > - INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); > - 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 (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) { > struct fastrpc_session_ctx *last_sess; > struct fastrpc_session_ctx *dup_sess; > - unsigned long flags; > > spin_lock_irqsave(&data->lock, flags); > last_sess = &data->session[data->sesscount - 1]; [Severity: High] Since fastrpc_cb_devices_create() now populates data->session[] before fastrpc_device_register() exposes the ADSP device node via misc_register(), can userspace open the device before this ADSP session duplication block runs? If a concurrent fastrpc_device_open() -> fastrpc_session_alloc() runs in that window, it marks the last CB session in-use: drivers/misc/fastrpc.c:fastrpc_session_alloc() { spin_lock_irqsave(&cctx->lock, flags); for (i = 0; i < cctx->sesscount; i++) { if (!cctx->session[i].used && cctx->session[i].valid) { cctx->session[i].used = true; session = &cctx->session[i]; ... break; } } spin_unlock_irqrestore(&cctx->lock, flags); } When fastrpc_rpmsg_probe() then copies last_sess into the FASTRPC_DUP_SESSIONS slots with memcpy(dup_sess, last_sess, sizeof(*dup_sess)), dup_sess->used = true is copied into all 4 duplicate session entries. Because no fastrpc_user owns those duplicated entries, fastrpc_session_free() will only clear last_sess->used, leaving the 4 duplicated ADSP sessions permanently marked used = true and unavailable for the lifetime of the channel. Should the ADSP session duplication block also be moved before fastrpc_device_register()? > @@ -2673,23 +2686,27 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > if (data->secure_fdevice) > misc_deregister(&data->secure_fdevice->miscdev); > > +err_depopulate: > + /* > + * A registered device node may already have been opened, so a user > + * can hold a cctx reference. Drop only our reference instead of > + * freeing the context directly. > + */ > + spin_lock_irqsave(&data->lock, flags); > + data->rpdev = NULL; > + list_for_each_entry(user, &data->users, user) > + fastrpc_notify_users(user); > + spin_unlock_irqrestore(&data->lock, flags); > + > + fastrpc_cb_devices_destroy(rpdev); > + fastrpc_channel_ctx_put(data); > + return err; > + > err_free_data: > kfree(data); > return err; > } [Severity: High] In rpmsg_dev_probe(), the endpoint is created with fastrpc_rpmsg_callback() before fastrpc_rpmsg_probe() is called, and is only destroyed after fastrpc_rpmsg_probe() returns: drivers/rpmsg/rpmsg_core.c:rpmsg_dev_probe() { ... ept = rpmsg_create_ept(rpdev, rpdrv->callback, NULL, chinfo); ... err = rpdrv->probe(rpdev); if (err) { dev_err(dev, "%s: failed: %d\n", __func__, err); goto destroy_ept; } } With dev_set_drvdata(&rpdev->dev, data) and rdev->dma_mask = &data->dma_mask moved ahead of fastrpc_cb_devices_create() and fastrpc_device_register(), both err_depopulate and err_free_data can free data while rpdev->dev driver_data and rdev->dma_mask still point to it. If an incoming glink interrupt runs fastrpc_rpmsg_callback() before rpmsg_dev_probe() destroys the endpoint, won't dev_get_drvdata(&rpdev->dev) return the freed data pointer, bypass the !cctx check, and dereference &cctx->lock and &cctx->ctx_idr after free? Also, in the err_free_data path, should idr_destroy(&data->ctx_idr) be called before kfree(data)? >[ ... ] 在进行最终输出前，检查结尾空行。 Let's remove the trailing `[ ... ]` and commentary note from the output. Everything is clean and ready. End of thought.commit ea9ec3521b0228bbc7931e61d1e5ec984d8cfeaf Author: Vinayak Katoch <[email protected]> misc: fastrpc: fix init ordering race in rpmsg probe This patch moves channel context initialization and fastrpc_cb_devices_create() ahead of fastrpc_device_register() in fastrpc_rpmsg_probe() so the context is initialized before device nodes are exposed to userspace. It also updates the probe error path to clean up compute-cb devices and drop the channel context reference. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f280bff2edf05..fc09cd27af4b8 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2605,6 +2619,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > data->poll_mode_supported = soc_data->poll_mode_supported || > of_machine_get_match(fastrpc_poll_supported_machines); > > + kref_init(&data->refcount); > + atomic_set(&data->ctx_seq, 0); > + > + rdev->dma_mask = &data->dma_mask; > + dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); > + INIT_LIST_HEAD(&data->users); > + INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); > + 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_free_data; > + > switch (domain_id) { > case ADSP_DOMAIN_ID: > case MDSP_DOMAIN_ID: > @@ -2613,7 +2644,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > data->unsigned_support = false; > err = fastrpc_device_register(rdev, data, secure_dsp, domain); > if (err) > - goto err_free_data; > + goto err_depopulate; > break; > case CDSP_DOMAIN_ID: > case GDSP_DOMAIN_ID: > @@ -2621,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > /* Create both device nodes so that we can allow both Signed > and Unsigned PD */ > err = fastrpc_device_register(rdev, data, true, domain); > if (err) > - goto err_free_data; > + goto err_depopulate; > > err = fastrpc_device_register(rdev, data, false, domain); > if (err) > @@ -2629,30 +2660,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > break; > default: > err = -EINVAL; > - goto err_free_data; > + goto err_depopulate; > } > > - kref_init(&data->refcount); > - atomic_set(&data->ctx_seq, 0); > - > - rdev->dma_mask = &data->dma_mask; > - dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); > - INIT_LIST_HEAD(&data->users); > - INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); > - 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 (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) { > struct fastrpc_session_ctx *last_sess; > struct fastrpc_session_ctx *dup_sess; > - unsigned long flags; > > spin_lock_irqsave(&data->lock, flags); > last_sess = &data->session[data->sesscount - 1]; [Severity: High] Since fastrpc_cb_devices_create() now populates data->session[] before fastrpc_device_register() exposes the ADSP device node via misc_register(), can userspace open the device before this ADSP session duplication block runs? If a concurrent fastrpc_device_open() -> fastrpc_session_alloc() runs in that window, it marks the last CB session in-use: drivers/misc/fastrpc.c:fastrpc_session_alloc() { spin_lock_irqsave(&cctx->lock, flags); for (i = 0; i < cctx->sesscount; i++) { if (!cctx->session[i].used && cctx->session[i].valid) { cctx->session[i].used = true; session = &cctx->session[i]; ... break; } } spin_unlock_irqrestore(&cctx->lock, flags); } When fastrpc_rpmsg_probe() then copies last_sess into the FASTRPC_DUP_SESSIONS slots with memcpy(dup_sess, last_sess, sizeof(*dup_sess)), dup_sess->used = true is copied into all 4 duplicate session entries. Because no fastrpc_user owns those duplicated entries, fastrpc_session_free() will only clear last_sess->used, leaving the 4 duplicated ADSP sessions permanently marked used = true and unavailable for the lifetime of the channel. Should the ADSP session duplication block also be moved before fastrpc_device_register()? > @@ -2673,23 +2686,27 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > if (data->secure_fdevice) > misc_deregister(&data->secure_fdevice->miscdev); > > +err_depopulate: > + /* > + * A registered device node may already have been opened, so a user > + * can hold a cctx reference. Drop only our reference instead of > + * freeing the context directly. > + */ > + spin_lock_irqsave(&data->lock, flags); > + data->rpdev = NULL; > + list_for_each_entry(user, &data->users, user) > + fastrpc_notify_users(user); > + spin_unlock_irqrestore(&data->lock, flags); > + > + fastrpc_cb_devices_destroy(rpdev); > + fastrpc_channel_ctx_put(data); > + return err; > + > err_free_data: > kfree(data); > return err; > } [Severity: High] In rpmsg_dev_probe(), the endpoint is created with fastrpc_rpmsg_callback() before fastrpc_rpmsg_probe() is called, and is only destroyed after fastrpc_rpmsg_probe() returns: drivers/rpmsg/rpmsg_core.c:rpmsg_dev_probe() { ... ept = rpmsg_create_ept(rpdev, rpdrv->callback, NULL, chinfo); ... err = rpdrv->probe(rpdev); if (err) { dev_err(dev, "%s: failed: %d\n", __func__, err); goto destroy_ept; } } With dev_set_drvdata(&rpdev->dev, data) and rdev->dma_mask = &data->dma_mask moved ahead of fastrpc_cb_devices_create() and fastrpc_device_register(), both err_depopulate and err_free_data can free data while rpdev->dev driver_data and rdev->dma_mask still point to it. If an incoming glink interrupt runs fastrpc_rpmsg_callback() before rpmsg_dev_probe() destroys the endpoint, won't dev_get_drvdata(&rpdev->dev) return the freed data pointer, bypass the !cctx check, and dereference &cctx->lock and &cctx->ctx_idr after free? Also, in the err_free_data path, should idr_destroy(&data->ctx_idr) be called before kfree(data)? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
