* [PATCH v2 0/3] virtio reset/device removal fixes
@ 2022-01-14 21:57 Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel
This fixes an issue found by stress-testing device removal
of virtio console, as well as a similar issue found by
code review in virtio mem.
Changes from v1:
- added documentation
- added virtio mem changes
- missing new line in virtio console change
v1 Link: https://lore.kernel.org/r/20211005070354.265164-1-mst@redhat.com
Michael S. Tsirkin (3):
virtio: document virtio_reset_device
virtio_console: break out of buf poll on remove
virtio_mem: break device on remove
drivers/char/virtio_console.c | 7 +++++++
drivers/virtio/virtio.c | 16 ++++++++++++++++
drivers/virtio/virtio_mem.c | 2 ++
3 files changed, 25 insertions(+)
--
MST
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/3] virtio: document virtio_reset_device
2022-01-14 21:57 [PATCH v2 0/3] virtio reset/device removal fixes Michael S. Tsirkin
@ 2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: Jason Wang, virtualization
Looks like most callers get driver/device removal wrong.
Document what's expected of callers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 2ed6e2451fd8..631a346a3aa6 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -201,6 +201,22 @@ static int virtio_finalize_features(struct virtio_device *dev)
return 0;
}
+/**
+ * virtio_reset_device - quiesce device for removal
+ * @dev: the device to reset
+ *
+ * Prevents device from sending interrupts and accessing memory.
+ *
+ * Generally used for cleanup during driver / device removal.
+ *
+ * Once this has been invoked, caller must ensure that
+ * virtqueue_notify / virtqueue_kick are not in progress.
+ *
+ * Note: this guarantees that vq callbacks are not in progress, however caller
+ * is responsible for preventing access from other contexts, such as a system
+ * call/workqueue/bh. Invoking virtio_break_device then flushing any such
+ * contexts is one way to handle that.
+ * */
void virtio_reset_device(struct virtio_device *dev)
{
dev->config->reset(dev);
--
MST
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 1/3] virtio: document virtio_reset_device
@ 2022-01-14 21:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization
Looks like most callers get driver/device removal wrong.
Document what's expected of callers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 2ed6e2451fd8..631a346a3aa6 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -201,6 +201,22 @@ static int virtio_finalize_features(struct virtio_device *dev)
return 0;
}
+/**
+ * virtio_reset_device - quiesce device for removal
+ * @dev: the device to reset
+ *
+ * Prevents device from sending interrupts and accessing memory.
+ *
+ * Generally used for cleanup during driver / device removal.
+ *
+ * Once this has been invoked, caller must ensure that
+ * virtqueue_notify / virtqueue_kick are not in progress.
+ *
+ * Note: this guarantees that vq callbacks are not in progress, however caller
+ * is responsible for preventing access from other contexts, such as a system
+ * call/workqueue/bh. Invoking virtio_break_device then flushing any such
+ * contexts is one way to handle that.
+ * */
void virtio_reset_device(struct virtio_device *dev)
{
dev->config->reset(dev);
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] virtio_console: break out of buf poll on remove
2022-01-14 21:57 [PATCH v2 0/3] virtio reset/device removal fixes Michael S. Tsirkin
@ 2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization
A common pattern for device reset is currently:
vdev->config->reset(vdev);
.. cleanup ..
reset prevents new interrupts from arriving and waits for interrupt
handlers to finish.
However if - as is common - the handler queues a work request which is
flushed during the cleanup stage, we have code adding buffers / trying
to get buffers while device is reset. Not good.
This was reproduced by running
modprobe virtio_console
modprobe -r virtio_console
in a loop.
Fix this up by calling virtio_break_device + flush before reset.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1786239
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/char/virtio_console.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2359889a35a0..e3c430539a17 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1957,6 +1957,13 @@ static void virtcons_remove(struct virtio_device *vdev)
list_del(&portdev->list);
spin_unlock_irq(&pdrvdata_lock);
+ /* Device is going away, exit any polling for buffers */
+ virtio_break_device(vdev);
+ if (use_multiport(portdev))
+ flush_work(&portdev->control_work);
+ else
+ flush_work(&portdev->config_work);
+
/* Disable interrupts for vqs */
virtio_reset_device(vdev);
/* Finish up work that's lined up */
--
MST
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] virtio_console: break out of buf poll on remove
@ 2022-01-14 21:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: Greg Kroah-Hartman, virtualization, Arnd Bergmann, Amit Shah
A common pattern for device reset is currently:
vdev->config->reset(vdev);
.. cleanup ..
reset prevents new interrupts from arriving and waits for interrupt
handlers to finish.
However if - as is common - the handler queues a work request which is
flushed during the cleanup stage, we have code adding buffers / trying
to get buffers while device is reset. Not good.
This was reproduced by running
modprobe virtio_console
modprobe -r virtio_console
in a loop.
Fix this up by calling virtio_break_device + flush before reset.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1786239
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/char/virtio_console.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2359889a35a0..e3c430539a17 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1957,6 +1957,13 @@ static void virtcons_remove(struct virtio_device *vdev)
list_del(&portdev->list);
spin_unlock_irq(&pdrvdata_lock);
+ /* Device is going away, exit any polling for buffers */
+ virtio_break_device(vdev);
+ if (use_multiport(portdev))
+ flush_work(&portdev->control_work);
+ else
+ flush_work(&portdev->config_work);
+
/* Disable interrupts for vqs */
virtio_reset_device(vdev);
/* Finish up work that's lined up */
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] virtio_mem: break device on remove
2022-01-14 21:57 [PATCH v2 0/3] virtio reset/device removal fixes Michael S. Tsirkin
@ 2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: David Hildenbrand, Jason Wang, virtualization
A common pattern for device reset is currently:
vdev->config->reset(vdev);
.. cleanup ..
reset prevents new interrupts from arriving and waits for interrupt
handlers to finish.
However if - as is common - the handler queues a work request which is
flushed during the cleanup stage, we have code adding buffers / trying
to get buffers while device is reset. Not good.
This was reproduced by running
modprobe virtio_console
modprobe -r virtio_console
in a loop, and this reasoning seems to apply to virtio mem though
I could not reproduce it there.
Fix this up by calling virtio_break_device + flush before reset.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_mem.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 38becd8d578c..33b8a118a3ae 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2888,6 +2888,8 @@ static void virtio_mem_remove(struct virtio_device *vdev)
virtio_mem_deinit_hotplug(vm);
/* reset the device and cleanup the queues */
+ virtio_break_device(vdev);
+ flush_work(&vm->wq);
virtio_reset_device(vdev);
vdev->config->del_vqs(vdev);
--
MST
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] virtio_mem: break device on remove
@ 2022-01-14 21:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-01-14 21:57 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization
A common pattern for device reset is currently:
vdev->config->reset(vdev);
.. cleanup ..
reset prevents new interrupts from arriving and waits for interrupt
handlers to finish.
However if - as is common - the handler queues a work request which is
flushed during the cleanup stage, we have code adding buffers / trying
to get buffers while device is reset. Not good.
This was reproduced by running
modprobe virtio_console
modprobe -r virtio_console
in a loop, and this reasoning seems to apply to virtio mem though
I could not reproduce it there.
Fix this up by calling virtio_break_device + flush before reset.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_mem.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 38becd8d578c..33b8a118a3ae 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2888,6 +2888,8 @@ static void virtio_mem_remove(struct virtio_device *vdev)
virtio_mem_deinit_hotplug(vm);
/* reset the device and cleanup the queues */
+ virtio_break_device(vdev);
+ flush_work(&vm->wq);
virtio_reset_device(vdev);
vdev->config->del_vqs(vdev);
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-14 21:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 21:57 [PATCH v2 0/3] virtio reset/device removal fixes Michael S. Tsirkin
2022-01-14 21:57 ` [PATCH v2 1/3] virtio: document virtio_reset_device Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` [PATCH v2 2/3] virtio_console: break out of buf poll on remove Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
2022-01-14 21:57 ` [PATCH v2 3/3] virtio_mem: break device " Michael S. Tsirkin
2022-01-14 21:57 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.