Virtualization - Jul 2021 - [PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization

On Sat, Jul 17, 2021 at 10:42:55AM +0300, Parav Pandit
wrote:
> 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.

I'm inclined to not include a Fixes
tag then. And please do change subject to say "theoretical"
to make that clear to people.
 
> Signed-off-by: Parav Pandit <parav at nvidia.com>
> ---
>  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?

The commit log does not describe this either.
>  	}
>  }
>  EXPORT_SYMBOL_GPL(virtio_break_device);
> -- 
> 2.27.0

> From: Michael S. Tsirkin <mst at redhat.com>
> Sent: Sunday, July 18, 2021 2:09 AM
> 
> On Sat, Jul 17, 2021 at 10:42:55AM +0300, Parav Pandit wrote:
> > 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.
> 
> 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.
Do you mean theoretical, because we haven't hit this bug?
> > Signed-off-by: Parav Pandit <parav at nvidia.com>
> > ---
> >  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.

> 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".
I will send v2 with 
(a) updated comment
(b) replacing smp.. with WRITE_ONCE()
(c) dropping the fixes tag.
> 
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(virtio_break_device);
> > --
> > 2.27.0