From: Michael S. Tsirkin <mst@redhat.com>
Sent: Monday, July 19, 2021 5:35 PM

On Mon, Jul 19, 2021 at 05:26:22AM +0000, Parav Pandit wrote:

quoted

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: Sunday, July 18, 2021 2:09 AM

On Sat, Jul 17, 2021 at 10:42:55AM +0300, Parav Pandit wrote:

quoted

Currently vq->broken field is read by virtqueue_is_broken() in
busy loop in one context by virtnet_send_command().

vq->broken is set to true in other process context by
virtio_break_device(). Reader and writer are accessing it without
any synchronization. This may lead to a compiler optimization
which may result to optimize reading vq->broken only once.

Hence, force reading vq->broken on each invocation of
virtqueue_is_broken() and ensure that its update is visible.

Fixes: e2dcdfe95c0b ("virtio: virtio_break_device() to mark all
virtqueues broken.")

This is all theoretical right?
virtqueue_get_buf is not inlined so compiler generally assumes any
vq field can change.

Broken bit checking cannot rely on some other kernel API for correctness.
So it possibly not hitting this case now, but we shouldn't depend other APIs

usage to ensure correctness.

quoted

quoted

I'm inclined to not include a Fixes
tag then. And please do change subject to say "theoretical"
to make that clear to people.

I do not have any strong opinion on fixes tag. But virtio_broken_queue()

API should be self-contained; for that I am not sure if this just theoretical.

quoted

Do you mean theoretical, because we haven't hit this bug?

Because with existing code I don't believe existing compilers are clever
enough to optimize this away.

Ok. got it. I will mention in the commit log.

quoted

quoted

quoted

Signed-off-by: Parav Pandit <redacted>
---
 drivers/virtio/virtio_ring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c

b/drivers/virtio/virtio_ring.c index 89bfe46a8a7f..7f379fe7d78d
100644

--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -2373,7 +2373,7 @@ bool virtqueue_is_broken(struct virtqueue

*_vq) {
 	struct vring_virtqueue *vq = to_vvq(_vq);

-	return vq->broken;
+	return READ_ONCE(vq->broken);
 }
 EXPORT_SYMBOL_GPL(virtqueue_is_broken);

@@ -2387,7 +2387,9 @@ void virtio_break_device(struct

virtio_device
*dev)

 	list_for_each_entry(_vq, &dev->vqs, list) {
 		struct vring_virtqueue *vq = to_vvq(_vq);
-		vq->broken = true;
+
+		/* Pairs with READ_ONCE() in virtqueue_is_broken(). */
+		smp_store_release(&vq->broken, true);

A bit puzzled here. Why do we need release semantics here?
IUC store_release does not generally pair with READ_ONCE - does it?

It does; paired at few places, such as,

(a) in uverbs_main.c, default_async_file
(b) in netlink.c, cb_table
(c) fs/dcache.c i_dir_seq,

However, in this scenario, WRITE_ONCE() is enough. So I will simplify and

use that in v1.

quoted

quoted

The commit log does not describe this either.

In commit log I mentioned that "ensure that update is visible".
I think a better commit log is, to say: "ensure that broken bit is written".

"is read repeatedly" maybe.

I updated it to below in v2.

" Hence, force reading vq->broken on each invocation of
virtqueue_is_broken() and also force writing it so that such update is visible to the readers."


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization