Re: [PATCH v3 3/6] remoteproc: implement synchronize_cbs() for virtio devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] idr_find() is called without rcu_read_lock() in preemptible contexts, leading to potential use-after-free of radix tree nodes.
- [High] The virtqueue pointer is published before its initialization is complete and without necessary memory barriers, exposing uninitialized memory to concurrent readers.
- [High] The added SRCU synchronization leaves the rvring structure vulnerable to Use-After-Free during device teardown.
- [High] Error path in rp_find_vq() erroneously removes the vring IDR entry without clearing it, permanently breaking the vring and causing a double-free on teardown.
- [High] rproc_add() error path causes a use-after-free and a double-free of the rproc structure.
- [High] Missing cancel_work_sync() for the crash handler work item before freeing the rproc struct.
--

commit c12071a358959f8d763c10c9e9b78779109dae3f
Author: Karl Mehltretter <kmehltretter@gmail.com>

remoteproc: implement synchronize_cbs() for virtio devices

Platform drivers invoke rproc_vq_interrupt() from hard-IRQ handlers,
threaded handlers, and work items. Because rpmsg callbacks may sleep,
the virtio core's synchronize_rcu() fallback does not synchronize with
callbacks across all these contexts. A device reset can therefore
complete while a callback is still running.

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f003be006b1bf..6756f2fe4ec50 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2367,6 +2367,7 @@ static void rproc_type_release(struct device *dev)
>  
>  	dev_info(&rproc->dev, "releasing %s\n", rproc->name);
>  
> +	cleanup_srcu_struct(&rproc->vq_srcu);

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a
use-after-free here if the crash handler work item is still pending?

rproc_alloc() initializes the rproc->crash_handler work item. If the remote
processor is deleted and freed, neither rproc_del() nor rproc_type_release()
calls cancel_work_sync(&rproc->crash_handler). If the work executes after this
structure is freed, the thread will dereference freed memory.

[Severity: High]
This is a pre-existing issue, but can the error path in rproc_add() cause a
use-after-free and a subsequent double-free?

If device_add(dev) fails, the error path erroneously calls put_device(dev):

drivers/remoteproc/remoteproc_core.c:rproc_add() {
    ...
    ret = device_add(dev);
    if (ret < 0) {
        put_device(dev);
        goto rproc_remove_cdev;
    }
    ...
rproc_remove_cdev:
    rproc_char_device_remove(rproc);
    return ret;
}

Since the initial refcount is 1, put_device() drops it to 0, immediately
freeing the rproc structure. Calling rproc_char_device_remove(rproc) then
accesses the cdev within the already freed memory. Returning the error to the
caller typically results in rproc_free() being called, causing a double-free.

>  	idr_destroy(&rproc->notifyids);
>  
>  	if (rproc->index >= 0)
[ ... ]
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index d5e9ff045a28a..7fefb4bd7adcb 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
[ ... ]
> @@ -88,15 +89,23 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
>   */
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
>  {
> +	irqreturn_t ret = IRQ_NONE;
>  	struct rproc_vring *rvring;
> +	struct virtqueue *vq;
> +	int idx;
>  
>  	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
>  
> +	idx = srcu_read_lock(&rproc->vq_srcu);
> +
>  	rvring = idr_find(&rproc->notifyids, notifyid);

[Severity: High]
This is a pre-existing issue, but might calling idr_find() here without
standard RCU read-side protection risk a use-after-free of internal radix tree
nodes?

This function runs in a preemptible context (threaded IRQ handler). idr_find()
relies on rcu_read_lock() for safe lockless traversal. While this patch wraps
it in srcu_read_lock(), SRCU does not block standard RCU grace periods.

If a concurrent idr_remove() runs (e.g., during device teardown) and a standard
RCU grace period completes while the thread is preempted inside idr_find(), the
internal nodes could be freed and then dereferenced when the thread resumes.

> -	if (!rvring || !rvring->vq)
> -		return IRQ_NONE;
> +	vq = rvring ? READ_ONCE(rvring->vq) : NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does the added SRCU
synchronization leave the rvring structure vulnerable to a use-after-free
during device teardown?

While rproc_vq_interrupt() is now protected by an SRCU critical section, there
does not appear to be a corresponding synchronize_srcu() in the platform device
teardown path:

drivers/remoteproc/remoteproc_virtio.c:rproc_virtio_remove() {
    ...
    for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
        rvring = &rvdev->vring[id];
        rproc_free_vring(rvring);
    }
    ...
}

The IDR entries are removed, but the function returns without waiting for SRCU
readers. The core driver then cleans up devres, instantly freeing the rvdev
(which embeds rvring). If rproc_vq_interrupt() is preempted after locating
rvring but before accessing rvring->vq, it will dereference freed memory upon
resumption.

> +	if (vq)
> +		ret = vring_interrupt(0, vq);
>  
> -	return vring_interrupt(0, rvring->vq);
> +	srcu_read_unlock(&rproc->vq_srcu, idx);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_vq_interrupt);
>  
> @@ -153,7 +162,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
>  
>  	vq->num_max = num;
>  
> -	rvring->vq = vq;
> +	WRITE_ONCE(rvring->vq, vq);
>  	vq->priv = rvring;

[Severity: High]
This isn't a bug introduced by this patch, but would publishing the virtqueue
pointer here without a release barrier expose uninitialized memory to
concurrent readers?

In vring_new_virtqueue(), vq is allocated and initialized, but vq->priv is not
set. The pointer is then published to concurrent readers via WRITE_ONCE.

Since WRITE_ONCE does not provide release semantics on weakly ordered
architectures, a concurrent rproc_vq_interrupt() could observe a non-NULL vq
before vq->priv is set or before the internal state of vq is visible. The
handler would then call vring_interrupt(0, vq) on an improperly initialized
structure.

[Severity: High]
This is a pre-existing issue, but could the error path in rp_find_vq()
permanently break the vring and cause a double-free on teardown?

If vring_new_virtqueue() fails, the error path calls rproc_free_vring(rvring),
which executes idr_remove(), but it does not reset rvring->notifyid:

drivers/remoteproc/remoteproc_virtio.c:rp_find_vq() {
    ...
    vq = vring_new_virtqueue(...);
    if (!vq) {
        dev_err(dev, "vring_new_virtqueue %s failed\n", name);
        rproc_free_vring(rvring);
        return ERR_PTR(-ENOMEM);
    }
    ...
}

If the driver successfully binds later, rp_find_vq() might succeed, but the
notifyid is no longer in the IDR, silently breaking interrupts. Furthermore,
when the device is unbound, rproc_virtio_remove() loops over all vrings and
calls rproc_free_vring() again, executing a second idr_remove() for the same
notifyid. If the ID was reused by another vring, its entry will be erroneously
deleted.

>  
>  	/* Update vring in resource table */
> @@ -165,14 +174,20 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=3