* [PATCH v2 2/2] virtio_net: fix race in RX VQ processing
2013-07-09 5:12 [PATCH v2 0/2] virtio_net: fix race in RX VQ processing Michael S. Tsirkin
@ 2013-07-09 5:13 ` Michael S. Tsirkin
2013-07-09 9:32 ` [PATCH v2 0/2] " David Miller
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-07-09 5:13 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, virtualization
virtio net called virtqueue_enable_cq on RX path after napi_complete, so
with NAPI_STATE_SCHED clear - outside the implicit napi lock.
This violates the requirement to synchronize virtqueue_enable_cq wrt
virtqueue_add_buf. In particular, used event can move backwards,
causing us to lose interrupts.
In a debug build, this can trigger panic within START_USE.
Jason Wang reports that he can trigger the races artificially,
by adding udelay() in virtqueue_enable_cb() after virtio_mb().
However, we must call napi_complete to clear NAPI_STATE_SCHED before
polling the virtqueue for used buffers, otherwise napi_schedule_prep in
a callback will fail, causing us to lose RX events.
To fix, call virtqueue_enable_cb_prepare with NAPI_STATE_SCHED
set (under napi lock), later call virtqueue_poll with
NAPI_STATE_SCHED clear (outside the lock).
Reported-by: Jason Wang <jasowang@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5305bd1..27f79dd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -605,7 +605,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
container_of(napi, struct receive_queue, napi);
struct virtnet_info *vi = rq->vq->vdev->priv;
void *buf;
- unsigned int len, received = 0;
+ unsigned int r, len, received = 0;
again:
while (received < budget &&
@@ -622,8 +622,9 @@ again:
/* Out of packets? */
if (received < budget) {
+ r = virtqueue_enable_cb_prepare(rq->vq);
napi_complete(napi);
- if (unlikely(!virtqueue_enable_cb(rq->vq)) &&
+ if (unlikely(virtqueue_poll(rq->vq, r)) &&
napi_schedule_prep(napi)) {
virtqueue_disable_cb(rq->vq);
__napi_schedule(napi);
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/2] virtio_net: fix race in RX VQ processing
2013-07-09 5:12 [PATCH v2 0/2] virtio_net: fix race in RX VQ processing Michael S. Tsirkin
2013-07-09 5:13 ` [PATCH v2 2/2] " Michael S. Tsirkin
@ 2013-07-09 9:32 ` David Miller
2013-07-09 10:21 ` Michael S. Tsirkin
2013-07-09 10:19 ` [PATCH v2 1/2] virtio: support unlocked queue poll Michael S. Tsirkin
2013-07-09 19:46 ` [PATCH v2 0/2] virtio_net: fix race in RX VQ processing David Miller
3 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-07-09 9:32 UTC (permalink / raw)
To: mst; +Cc: linux-kernel, netdev, rusty, jasowang
I don't see patch #1 in v2 of this series.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] virtio: support unlocked queue poll
2013-07-09 5:12 [PATCH v2 0/2] virtio_net: fix race in RX VQ processing Michael S. Tsirkin
2013-07-09 5:13 ` [PATCH v2 2/2] " Michael S. Tsirkin
2013-07-09 9:32 ` [PATCH v2 0/2] " David Miller
@ 2013-07-09 10:19 ` Michael S. Tsirkin
2013-07-09 19:46 ` [PATCH v2 0/2] virtio_net: fix race in RX VQ processing David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-07-09 10:19 UTC (permalink / raw)
To: linux-kernel; +Cc: Jason Wang, Rusty Russell, virtualization, davem, netdev
This adds a way to check ring empty state after enable_cb outside any
locks. Will be used by virtio_net.
Note: there's room for more optimization: caller is likely to have a
memory barrier already, which means we might be able to get rid of a
barrier here. Deferring this optimization until we do some
benchmarking.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 56 ++++++++++++++++++++++++++++++++++----------
include/linux/virtio.h | 4 ++++
2 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5217baf..37d58f8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -607,19 +607,21 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
/**
- * virtqueue_enable_cb - restart callbacks after disable_cb.
+ * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
* @vq: the struct virtqueue we're talking about.
*
- * This re-enables callbacks; it returns "false" if there are pending
- * buffers in the queue, to detect a possible race between the driver
- * checking for more work, and enabling callbacks.
+ * This re-enables callbacks; it returns current queue state
+ * in an opaque unsigned value. This value should be later tested by
+ * virtqueue_poll, to detect a possible race between the driver checking for
+ * more work, and enabling callbacks.
*
* Caller must ensure we don't call this with other virtqueue
* operations at the same time (except where noted).
*/
-bool virtqueue_enable_cb(struct virtqueue *_vq)
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 last_used_idx;
START_USE(vq);
@@ -629,15 +631,45 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
* either clear the flags bit or point the event index at the next
* entry. Always do both to keep code simple. */
vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
- vring_used_event(&vq->vring) = vq->last_used_idx;
+ vring_used_event(&vq->vring) = last_used_idx = vq->last_used_idx;
+ END_USE(vq);
+ return last_used_idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare);
+
+/**
+ * virtqueue_poll - query pending used buffers
+ * @vq: the struct virtqueue we're talking about.
+ * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare).
+ *
+ * Returns "true" if there are pending used buffers in the queue.
+ *
+ * This does not need to be serialized.
+ */
+bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
virtio_mb(vq->weak_barriers);
- if (unlikely(more_used(vq))) {
- END_USE(vq);
- return false;
- }
+ return (u16)last_used_idx != vq->vring.used->idx;
+}
+EXPORT_SYMBOL_GPL(virtqueue_poll);
- END_USE(vq);
- return true;
+/**
+ * virtqueue_enable_cb - restart callbacks after disable_cb.
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * This re-enables callbacks; it returns "false" if there are pending
+ * buffers in the queue, to detect a possible race between the driver
+ * checking for more work, and enabling callbacks.
+ *
+ * Caller must ensure we don't call this with other virtqueue
+ * operations at the same time (except where noted).
+ */
+bool virtqueue_enable_cb(struct virtqueue *_vq)
+{
+ unsigned last_used_idx = virtqueue_enable_cb_prepare(_vq);
+ return !virtqueue_poll(_vq, last_used_idx);
}
EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 9ff8645..72398ee 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -70,6 +70,10 @@ void virtqueue_disable_cb(struct virtqueue *vq);
bool virtqueue_enable_cb(struct virtqueue *vq);
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
+
+bool virtqueue_poll(struct virtqueue *vq, unsigned);
+
bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
void *virtqueue_detach_unused_buf(struct virtqueue *vq);
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/2] virtio_net: fix race in RX VQ processing
2013-07-09 5:12 [PATCH v2 0/2] virtio_net: fix race in RX VQ processing Michael S. Tsirkin
` (2 preceding siblings ...)
2013-07-09 10:19 ` [PATCH v2 1/2] virtio: support unlocked queue poll Michael S. Tsirkin
@ 2013-07-09 19:46 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-07-09 19:46 UTC (permalink / raw)
To: mst; +Cc: linux-kernel, netdev, rusty, jasowang
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 9 Jul 2013 08:12:57 +0300
> Jason Wang reported a race in RX VQ processing:
> virtqueue_enable_cb is called outside napi lock,
> violating virtio serialization rules.
> The race has been there from day 1, but it got especially nasty in 3.0
> when commit a5c262c5fd83ece01bd649fb08416c501d4c59d7
> "virtio_ring: support event idx feature"
> added more dependency on vq state.
>
> Please review, and consider for 3.11 and stable.
>
> Changes from v1:
> - Added Jason's Tested-by tag
> - minor coding style fix
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread