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 <[email protected]> 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/[email protected]?part=1
