From: sashiko-bot@kernel.org
To: "Vinayak Katoch" <vinayak.katoch@oss.qualcomm.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] misc: fastrpc: fix init ordering race in rpmsg probe
Date: Thu, 22 Oct 2026 09:29:01 +0000 [thread overview]
Message-ID: <sashiko-outbox-181262@kernel.org> (raw)
In-Reply-To: <20261022-fastrpc-probe-fixes-v2-2-a6b3f6f62571@oss.qualcomm.com>
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 <vinayak.katoch@oss.qualcomm.com>
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 <vinayak.katoch@oss.qualcomm.com>
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/20261022-fastrpc-probe-fixes-v2-0-a6b3f6f62571@oss.qualcomm.com?part=2
prev parent reply other threads:[~2026-10-22 9:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-22 9:08 [PATCH v2 0/2] fastrpc: fix probe-time races between rpmsg and userspace Vinayak Katoch
2026-10-22 9:08 ` [PATCH v2 1/2] misc: fastrpc: fix NULL rpdev dereference when session alloc fails Vinayak Katoch
2026-10-22 9:08 ` [PATCH v2 2/2] misc: fastrpc: fix init ordering race in rpmsg probe Vinayak Katoch
2026-10-22 9:29 ` sashiko-bot [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-181262@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=sashiko-reviews@lists.linux.dev \
--cc=vinayak.katoch@oss.qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.