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

[sashiko-bot@kernel.org]

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