Parav Pandit
2021-Jul-19 05:26 UTC
[PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization
> 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
Michael S. Tsirkin
2021-Jul-19 12:04 UTC
[PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization
On Mon, Jul 19, 2021 at 05:26:22AM +0000, Parav Pandit wrote:> > > > 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?Because with existing code I don't believe existing compilers are clever enough to optimize this away.> > > 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"."is read repeatedly" maybe.> 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