On Sun, Nov 08, 2026 at 04:50:38PM +0000, [email protected] 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 <[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. >
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/[email protected]?part=1 Regards, Francesco
