[PATCH] virtio: check for driver remove callback before calling it

[PATCH] virtio: check for driver remove callback before calling it

[Francesco Valla]

The .remove() callback of a driver is normally not mandatory, and the
increase of devm_ functions availability makes it even less so. For a
virtio_driver it's not marked explicitly as optional, but nothing
enforces its presence, so check for its existence before calling it.

The NULL dereference that led to this fix was seen in the wild for the
spi-virtio driver, which is currently the only in-tree virtio driver
without a remove callback.

Fixes: 74b2553f1d13 ("virtio: fix module/device unloading")
Signed-off-by: Francesco Valla <francesco@valla.it>
---
The behavior of the spi-virtio driver is not conformant to the virtio
framework guidelines, but that will be tackled in a different series.
---
 drivers/virtio/virtio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a8588d7ad109..067d32c67b44 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -374,7 +374,8 @@ static void virtio_dev_remove(struct device *_d)
 
 	virtio_config_core_disable(dev);
 
-	drv->remove(dev);
+	if (drv->remove)
+		drv->remove(dev);
 
 	/* Driver should have reset device. */
 	WARN_ON_ONCE(dev->config->get_status(dev));

---
base-commit: fa4ad798f6f681031775266044ba1b0cf3c670bd
change-id: 20261108-virtio_drv_remove_fix-5ba8e3123d7e

Best regards,
--  
Francesco Valla <francesco@valla.it>

Re: [PATCH] virtio: check for driver remove callback before calling it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] virtio: unreset device in virtio_dev_remove() when
  drv->remove is NULL
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] virtio: use-after-free in virtio_dev_remove() [1]
--

commit ccd4078efa3bba738fda1cc44e8e8955495a84fa
Author: Francesco Valla <francesco@valla.it>

virtio: check for driver remove callback before calling it

This commit checks whether drv->remove is non-NULL before calling it in
virtio_dev_remove().

> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a8588d7ad109f..067d32c67b44f 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -374,7 +374,8 @@ static void virtio_dev_remove(struct device *_d)
>  
>  	virtio_config_core_disable(dev);
>  
> -	drv->remove(dev);
> +	if (drv->remove)
> +		drv->remove(dev);
>  
>  	/* Driver should have reset device. */
>  	WARN_ON_ONCE(dev->config->get_status(dev));

[Severity: Medium]
When drv->remove is NULL, will WARN_ON_ONCE(dev->config->get_status(dev))
always fire on unbind or module unload?

__device_release_driver() calls device_remove() (which runs
virtio_dev_remove()) before device_unbind_cleanup() runs
devres_release_all().  Even when a driver such as spi-virtio registers a
devm callback to reset the device and delete its virtqueues, that cleanup
has not run yet when virtio_dev_remove() checks get_status() and sets
VIRTIO_CONFIG_S_ACKNOWLEDGE.  The subsequent devm reset then wipes out
VIRTIO_CONFIG_S_ACKNOWLEDGE, or if a driver has no devm reset action at
all, the device is never reset and remains active with live virtqueues and
DMA after its devm resources are freed.

[Severity: Critical]
This problem wasn't introduced by this patch, but calling
of_node_put(dev->dev.of_node) at the end of virtio_dev_remove() causes a
refcount underflow and use-after-free [1].

virtio_device_of_init() acquires the dev->dev.of_node reference during
register_virtio_device(), not during driver probe, and virtio_dev_remove()
drops that reference on driver unbind without clearing dev->dev.of_node.
Unbinding and rebinding a driver drops the reference multiple times on the
same pointer, while unregistering a device that never bound to a driver
leaks the reference.

[1] https://sashiko.dev/#/bug/linux-cf8a6c4a-49fb-4827-8f82-d189cf36ccdd

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-virtio_drv_remove_fix-v1-1-23da1a7c3900@valla.it?part=1

Re: [PATCH] virtio: check for driver remove callback before calling it

[Francesco Valla]

On Sun, Nov 08, 2026 at 04:50:38PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] virtio: unreset device in virtio_dev_remove() when
>   drv->remove is NULL
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [Critical] virtio: use-after-free in virtio_dev_remove() [1]
> --
> 
> commit ccd4078efa3bba738fda1cc44e8e8955495a84fa
> Author: Francesco Valla <francesco@valla.it>
> 
> virtio: check for driver remove callback before calling it
> 
> This commit checks whether drv->remove is non-NULL before calling it in
> virtio_dev_remove().
> 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index a8588d7ad109f..067d32c67b44f 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -374,7 +374,8 @@ static void virtio_dev_remove(struct device *_d)
> >  
> >  	virtio_config_core_disable(dev);
> >  
> > -	drv->remove(dev);
> > +	if (drv->remove)
> > +		drv->remove(dev);
> >  
> >  	/* Driver should have reset device. */
> >  	WARN_ON_ONCE(dev->config->get_status(dev));
> 
> [Severity: Medium]
> When drv->remove is NULL, will WARN_ON_ONCE(dev->config->get_status(dev))
> always fire on unbind or module unload?
> 
> __device_release_driver() calls device_remove() (which runs
> virtio_dev_remove()) before device_unbind_cleanup() runs
> devres_release_all().  Even when a driver such as spi-virtio registers a
> devm callback to reset the device and delete its virtqueues, that cleanup
> has not run yet when virtio_dev_remove() checks get_status() and sets
> VIRTIO_CONFIG_S_ACKNOWLEDGE.  The subsequent devm reset then wipes out
> VIRTIO_CONFIG_S_ACKNOWLEDGE, or if a driver has no devm reset action at
> all, the device is never reset and remains active with live virtqueues and
> DMA after its devm resources are freed.
>

Yes, but that's (IMO) better than a NULL dereference. The spi-virtio
driver will be fixed in a different series, as stated.

> [Severity: Critical]
> This problem wasn't introduced by this patch, but calling
> of_node_put(dev->dev.of_node) at the end of virtio_dev_remove() causes a
> refcount underflow and use-after-free [1].
> 
> virtio_device_of_init() acquires the dev->dev.of_node reference during
> register_virtio_device(), not during driver probe, and virtio_dev_remove()
> drops that reference on driver unbind without clearing dev->dev.of_node.
> Unbinding and rebinding a driver drops the reference multiple times on the
> same pointer, while unregistering a device that never bound to a driver
> leaks the reference.
> 
> [1] https://sashiko.dev/#/bug/linux-cf8a6c4a-49fb-4827-8f82-d189cf36ccdd
> 

That's... interesting. I'll add it to my backlog if no one is already
working on it.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261108-virtio_drv_remove_fix-v1-1-23da1a7c3900@valla.it?part=1

Regards,
Francesco