RE: [PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization
> From: Michael S. Tsirkin
> Sent: Monday, July 19, 2021 5:35 PM
>
> On Mon, Jul 19, 2021 at 05:26:22AM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin
> > > 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.
Ok. got it. I will mention in the commit log.
>
> > > > Signed-off-by: Parav Pandit
> > > > ---
> > > > 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, >vqs, list) {
> > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > > - vq->broken = true;
> > > > +
> > > > + /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
> > > > + smp_store_release(>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 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
Re: [PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization
On Mon, Jul 19, 2021 at 05:26:22AM +, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin
> > 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
> > > ---
> > > 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, >vqs, list) {
> > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > - vq->broken = true;
> > > +
> > > + /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
> > > + smp_store_release(>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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/4] virtio: Improve vq->broken access to avoid any compiler optimization
> From: Michael S. Tsirkin
> 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
> > ---
> > 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, >vqs, list) {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > - vq->broken = true;
> > +
> > + /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
> > + smp_store_release(>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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [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
> ---
> 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, >vqs, list) {
> struct vring_virtqueue *vq = to_vvq(_vq);
> - vq->broken = true;
> +
> + /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
> + smp_store_release(>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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
