| Message ID | 1479327452-16096-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > Sent: Thursday, November 17, 2016 4:18 AM > To: qemu-devel@nongnu.org > Cc: Gonglei (Arei); Michael S. Tsirkin; Stefan Hajnoczi > Subject: [PATCH] virtio-crypto: fix virtio_queue_set_notification() race > > We must check for new virtqueue buffers after re-enabling notifications. > This prevents the race condition where the guest added buffers just > after we stopped popping the virtqueue but before we re-enabled > notifications. > > I think the virtio-crypto code was based on virtio-net but this crucial > detail was missed. virtio-net does not have the race condition because > it processes the virtqueue one more time after re-enabling > notifications. > > Cc: Gonglei <arei.gonglei@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/virtio/virtio-crypto.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > Reviewed-by: Gonglei <arei.gonglei@huawei.com> Thanks, -Gonglei > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c > index 3293843..847dc9d 100644 > --- a/hw/virtio/virtio-crypto.c > +++ b/hw/virtio/virtio-crypto.c > @@ -692,8 +692,17 @@ static void virtio_crypto_dataq_bh(void *opaque) > return; > } > > - virtio_crypto_handle_dataq(vdev, q->dataq); > - virtio_queue_set_notification(q->dataq, 1); > + for (;;) { > + virtio_crypto_handle_dataq(vdev, q->dataq); > + virtio_queue_set_notification(q->dataq, 1); > + > + /* Are we done or did the guest add more buffers? */ > + if (virtio_queue_empty(q->dataq)) { > + break; > + } > + > + virtio_queue_set_notification(q->dataq, 0); > + } > } > > static void > -- > 2.7.4
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c index 3293843..847dc9d 100644 --- a/hw/virtio/virtio-crypto.c +++ b/hw/virtio/virtio-crypto.c @@ -692,8 +692,17 @@ static void virtio_crypto_dataq_bh(void *opaque) return; } - virtio_crypto_handle_dataq(vdev, q->dataq); - virtio_queue_set_notification(q->dataq, 1); + for (;;) { + virtio_crypto_handle_dataq(vdev, q->dataq); + virtio_queue_set_notification(q->dataq, 1); + + /* Are we done or did the guest add more buffers? */ + if (virtio_queue_empty(q->dataq)) { + break; + } + + virtio_queue_set_notification(q->dataq, 0); + } } static void
We must check for new virtqueue buffers after re-enabling notifications. This prevents the race condition where the guest added buffers just after we stopped popping the virtqueue but before we re-enabled notifications. I think the virtio-crypto code was based on virtio-net but this crucial detail was missed. virtio-net does not have the race condition because it processes the virtqueue one more time after re-enabling notifications. Cc: Gonglei <arei.gonglei@huawei.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/virtio/virtio-crypto.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)