Re: Merge of "virtio_net: fix race in RX VQ processing" for linux-3.2.48

Re: Merge of "virtio_net: fix race in RX VQ processing" for linux-3.2.48

[Ben Hutchings]

[-- Attachment #1.1.1: Type: text/plain, Size: 1158 bytes --]

On Fri, 2013-07-12 at 23:13 +0200, Wolfram Gloger wrote:
> Hi,
> 
> Today I merged M. Tsirkin's patch v2 "virtio_net: fix race in RX VQ
> processing":
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/1307.1/00503.html
> 
> into 3.2.48 and lightly tested the result (vhost-net, virtio-disk
> and 9pfs using virtio).

This sounds like it could be suitable for stable, but that doesn't seem
to have been requested by the author.  I'm cc'ing those involved so they
can make a decision whether this should be included in 3.2.y or other
stable branches.

> Merge is not completely trivial, so might save you some time, if only
> for a comparison that you arrive at the same result.

Thanks, but these are not in quite the right patch format.  The
filenames should always have a leading directory name which will be
stripped (patch -p1).  The original patch header must be preserved and a
reference to the upstream commit inserted e.g.
'commit 0123456789abcdef0123456789abcdef012345678 upstream.'

(Attachments copied for reference.)

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.


[-- Attachment #1.1.2: virtio-race-tsirkin-1of2.diff --]
[-- Type: text/x-patch, Size: 3257 bytes --]

--- drivers/virtio/virtio_ring.c.orig	2013-06-30 11:35:44.000000000 +0200
+++ drivers/virtio/virtio_ring.c	2013-07-12 15:18:48.000000000 +0200
@@ -360,9 +360,22 @@ void virtqueue_disable_cb(struct virtque
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
 
-bool virtqueue_enable_cb(struct virtqueue *_vq)
+/**
+ * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * 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).
+ */
+unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	u16 last_used_idx;
 
 	START_USE(vq);
 
@@ -372,15 +385,45 @@ bool virtqueue_enable_cb(struct virtqueu
 	 * 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();
-	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);
 
--- include/linux/virtio.h.orig	2012-01-05 00:55:44.000000000 +0100
+++ include/linux/virtio.h	2013-07-12 14:42:26.000000000 +0200
@@ -96,6 +96,10 @@ void virtqueue_disable_cb(struct virtque
 
 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);

[-- Attachment #1.1.3: virtio-race-tsirkin-2of2.diff --]
[-- Type: text/x-patch, Size: 763 bytes --]

--- drivers/net/virtio_net.c.orig	2012-01-05 00:55:44.000000000 +0100
+++ drivers/net/virtio_net.c	2013-07-12 15:39:06.000000000 +0200
@@ -508,7 +508,7 @@ static int virtnet_poll(struct napi_stru
 {
 	struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
 	void *buf;
-	unsigned int len, received = 0;
+	unsigned int r, len, received = 0;
 
 again:
 	while (received < budget &&
@@ -525,8 +525,9 @@ again:
 
 	/* Out of packets? */
 	if (received < budget) {
+		r = virtqueue_enable_cb_prepare(vi->rvq);
 		napi_complete(napi);
-		if (unlikely(!virtqueue_enable_cb(vi->rvq)) &&
+		if (unlikely(virtqueue_poll(vi->rvq, r)) &&
 		    napi_schedule_prep(napi)) {
 			virtqueue_disable_cb(vi->rvq);
 			__napi_schedule(napi);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Merge of "virtio_net: fix race in RX VQ processing" for linux-3.2.48

[Michael S. Tsirkin]

On Sat, Jul 27, 2013 at 03:42:53PM +0100, Ben Hutchings wrote:
> On Fri, 2013-07-12 at 23:13 +0200, Wolfram Gloger wrote:
> > Hi,
> > 
> > Today I merged M. Tsirkin's patch v2 "virtio_net: fix race in RX VQ
> > processing":
> > 
> > http://lkml.indiana.edu/hypermail/linux/kernel/1307.1/00503.html
> > 
> > into 3.2.48 and lightly tested the result (vhost-net, virtio-disk
> > and 9pfs using virtio).
> 
> This sounds like it could be suitable for stable, but that doesn't seem
> to have been requested by the author.

It was in the cover letter:
"Please review, and consider for 3.11 and stable."

> I'm cc'ing those involved so they
> can make a decision whether this should be included in 3.2.y or other
> stable branches.
> > Merge is not completely trivial, so might save you some time, if only
> > for a comparison that you arrive at the same result.
> 
> Thanks, but these are not in quite the right patch format.  The
> filenames should always have a leading directory name which will be
> stripped (patch -p1).  The original patch header must be preserved and a
> reference to the upstream commit inserted e.g.
> 'commit 0123456789abcdef0123456789abcdef012345678 upstream.'
> 
> (Attachments copied for reference.)
> 
> Ben.
> 
> -- 
> Ben Hutchings
> Once a job is fouled up, anything done to improve it makes it worse.
> 

> --- drivers/virtio/virtio_ring.c.orig	2013-06-30 11:35:44.000000000 +0200
> +++ drivers/virtio/virtio_ring.c	2013-07-12 15:18:48.000000000 +0200
> @@ -360,9 +360,22 @@ void virtqueue_disable_cb(struct virtque
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
>  
> -bool virtqueue_enable_cb(struct virtqueue *_vq)
> +/**
> + * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
> + * @vq: the struct virtqueue we're talking about.
> + *
> + * 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).
> + */
> +unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> +	u16 last_used_idx;
>  
>  	START_USE(vq);
>  
> @@ -372,15 +385,45 @@ bool virtqueue_enable_cb(struct virtqueu
>  	 * 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();
> -	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);
>  
> --- include/linux/virtio.h.orig	2012-01-05 00:55:44.000000000 +0100
> +++ include/linux/virtio.h	2013-07-12 14:42:26.000000000 +0200
> @@ -96,6 +96,10 @@ void virtqueue_disable_cb(struct virtque
>  
>  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);

> --- drivers/net/virtio_net.c.orig	2012-01-05 00:55:44.000000000 +0100
> +++ drivers/net/virtio_net.c	2013-07-12 15:39:06.000000000 +0200
> @@ -508,7 +508,7 @@ static int virtnet_poll(struct napi_stru
>  {
>  	struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
>  	void *buf;
> -	unsigned int len, received = 0;
> +	unsigned int r, len, received = 0;
>  
>  again:
>  	while (received < budget &&
> @@ -525,8 +525,9 @@ again:
>  
>  	/* Out of packets? */
>  	if (received < budget) {
> +		r = virtqueue_enable_cb_prepare(vi->rvq);
>  		napi_complete(napi);
> -		if (unlikely(!virtqueue_enable_cb(vi->rvq)) &&
> +		if (unlikely(virtqueue_poll(vi->rvq, r)) &&
>  		    napi_schedule_prep(napi)) {
>  			virtqueue_disable_cb(vi->rvq);
>  			__napi_schedule(napi);