Linux Virtualization - Nov 2013 - [PATCH RFC] virtio_net: fix error handling for mergeable buffers

When mergeable buffer were used, we only put the first page buf leave the rest
of buffers in the virt queue. This will cause the driver could not get the
correct head buffer any more. Fix this by dropping the rest of buffers for this
packet.

The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
(virtio_net: Defer skb allocation in receive path).

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: Michael Dalton <mwdalton at google.com>
Cc: Eric Dumazet <edumazet at google.com>
Cc: Shirley Ma <xma at us.ibm.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
This patch was needed for stable
---
 drivers/net/virtio_net.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7bab4de..24fd502 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
 	netif_wake_subqueue(vi->dev, vq2txq(vq));
 }
 
+static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
+{
+	char *buf;
+	int len;
+
+	while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) !=
NULL) {
+		--rq->num;
+		put_page(virt_to_head_page(buf));
+	}
+}
+
 /* Called from bottom half context */
 static struct sk_buff *page_to_skb(struct receive_queue *rq,
 				   struct page *page, unsigned int offset,
@@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct receive_queue
*rq,
 
 	/* copy small packet so we can reuse these pages for small data */
 	skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
-	if (unlikely(!skb))
+	if (unlikely(!skb)) {
+		if (vi->mergeable_rx_bufs) {
+			hdr = (struct skb_vnet_hdr *)p;
+			drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
+		}
 		return NULL;
+	}
 
 	hdr = skb_vnet_hdr(skb);
 
-- 
1.8.3.2

We need decrease the rq->num after we can get a buf through
virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
refill routine won't be triggered under heavy memory stress since the driver
may
still think there's enough room.

This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
(virtio_net: migrate mergeable rx buffers to page frag allocators).

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: Michael Dalton <mwdalton at google.com>
Cc: Eric Dumazet <edumazet at google.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
The patch was needed for 3.12 stable.
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 24fd502..de1d6ca 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -333,6 +333,7 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 			head_skb->dev->stats.rx_length_errors++;
 			return -EINVAL;
 		}
+		--rq->num;
 		if (unlikely(len > MERGE_BUFFER_LEN)) {
 			pr_debug("%s: rx error: merge buffer too long\n",
 				 head_skb->dev->name);
@@ -367,7 +368,6 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 			skb_add_rx_frag(curr_skb, num_skb_frags, page,
 					offset, len, MERGE_BUFFER_LEN);
 		}
-		--rq->num;
 	}
 	return 0;
 }
-- 
1.8.3.2

When we fail to allocate a frag skb, we need put the refcnt of the page and drop
the rest of the buffers. Otherwise the page was leaked and driver won't get
a
correct head buffer after this failure.

This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
(virtio_net: migrate mergeable rx buffers to page frag allocators).

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: Michael S. Tsirkin <mst at redhat.com>
Cc: Michael Dalton <mwdalton at google.com>
Cc: Eric Dumazet <edumazet at google.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
The patch was needed for 3.12 stable.
---
 drivers/net/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index de1d6ca..f6f1e20 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -339,9 +339,12 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 				 head_skb->dev->name);
 			len = MERGE_BUFFER_LEN;
 		}
+		page = virt_to_head_page(buf);
 		if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
 			struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
 			if (unlikely(!nskb)) {
+				put_page(page);
+				drop_mergeable_buffer(rq, num_buf);
 				head_skb->dev->stats.rx_dropped++;
 				return -ENOMEM;
 			}
@@ -358,7 +361,6 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 			head_skb->len += len;
 			head_skb->truesize += MERGE_BUFFER_LEN;
 		}
-		page = virt_to_head_page(buf);
 		offset = buf - (char *)page_address(page);
 		if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
 			put_page(page);
-- 
1.8.3.2

On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang
wrote:
> When mergeable buffer were used, we only put the first page buf leave the
rest
> of buffers in the virt queue. This will cause the driver could not get the
> correct head buffer any more. Fix this by dropping the rest of buffers for
this
> packet.
> 
> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> (virtio_net: Defer skb allocation in receive path).
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Michael Dalton <mwdalton at google.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Cc: Shirley Ma <xma at us.ibm.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
> This patch was needed for stable
> ---
>  drivers/net/virtio_net.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7bab4de..24fd502 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
>  	netif_wake_subqueue(vi->dev, vq2txq(vq));
>  }
>  
> +static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
> +{
> +	char *buf;
> +	int len;
> +
> +	while (--num_buf && (buf = virtqueue_get_buf(rq->vq,
&len)) != NULL) {
> +		--rq->num;
> +		put_page(virt_to_head_page(buf));
> +	}
> +}
> +
This is the same code we have in receive_mergeable anyway.
So let's reuse that.

>  /* Called from bottom half context */
>  static struct sk_buff *page_to_skb(struct receive_queue *rq,
>  				   struct page *page, unsigned int offset,
> @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct
receive_queue *rq,
>  
>  	/* copy small packet so we can reuse these pages for small data */
>  	skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> -	if (unlikely(!skb))
> +	if (unlikely(!skb)) {
> +		if (vi->mergeable_rx_bufs) {
> +			hdr = (struct skb_vnet_hdr *)p;
> +			drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> +		}
>  		return NULL;
> +	}
>  
>  	hdr = skb_vnet_hdr(skb);
>  
> -- 
> 1.8.3.2

On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang
wrote:
> We need decrease the rq->num after we can get a buf through
> virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
> refill routine won't be triggered under heavy memory stress since the
driver may
> still think there's enough room.
> 
> This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
> (virtio_net: migrate mergeable rx buffers to page frag allocators).
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Michael Dalton <mwdalton at google.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
So let's wrap virtqueue_get_buf to make sure we get it right?
> ---
> The patch was needed for 3.12 stable.
> ---
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 24fd502..de1d6ca 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -333,6 +333,7 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
>  			head_skb->dev->stats.rx_length_errors++;
>  			return -EINVAL;
>  		}
> +		--rq->num;
>  		if (unlikely(len > MERGE_BUFFER_LEN)) {
>  			pr_debug("%s: rx error: merge buffer too long\n",
>  				 head_skb->dev->name);
> @@ -367,7 +368,6 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
>  			skb_add_rx_frag(curr_skb, num_skb_frags, page,
>  					offset, len, MERGE_BUFFER_LEN);
>  		}
> -		--rq->num;
>  	}
>  	return 0;
>  }
> -- 
> 1.8.3.2

----- ???? -----
> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > When mergeable buffer were used, we only put the first page buf leave
the
> > rest
> > of buffers in the virt queue. This will cause the driver could not get
the
> > correct head buffer any more. Fix this by dropping the rest of buffers
for
> > this
> > packet.
> > 
> > The bug was introduced by commit
9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > (virtio_net: Defer skb allocation in receive path).
> > 
> > Cc: Rusty Russell <rusty at rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Cc: Michael Dalton <mwdalton at google.com>
> > Cc: Eric Dumazet <edumazet at google.com>
> > Cc: Shirley Ma <xma at us.ibm.com>
> > Signed-off-by: Jason Wang <jasowang at redhat.com>
> > ---
> > This patch was needed for stable
> > ---
> >  drivers/net/virtio_net.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7bab4de..24fd502 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
> >  	netif_wake_subqueue(vi->dev, vq2txq(vq));
> >  }
> >  
> > +static void drop_mergeable_buffer(struct receive_queue *rq, int
num_buf)
> > +{
> > +	char *buf;
> > +	int len;
> > +
> > +	while (--num_buf && (buf = virtqueue_get_buf(rq->vq,
&len)) != NULL) {
> > +		--rq->num;
> > +		put_page(virt_to_head_page(buf));
> > +	}
> > +}
> > +
> 
> This is the same code we have in receive_mergeable anyway.
> So let's reuse that.
> 
> 
receive_mergeable() was called after page_to_skb() was called and 
there's lots of conditions check there. I'm not sure how could we 
reuse them.
> >  /* Called from bottom half context */
> >  static struct sk_buff *page_to_skb(struct receive_queue *rq,
> >  				   struct page *page, unsigned int offset,
> > @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct
> > receive_queue *rq,
> >  
> >  	/* copy small packet so we can reuse these pages for small data */
> >  	skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> > -	if (unlikely(!skb))
> > +	if (unlikely(!skb)) {
> > +		if (vi->mergeable_rx_bufs) {
> > +			hdr = (struct skb_vnet_hdr *)p;
> > +			drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> > +		}
> >  		return NULL;
> > +	}
> >  
> >  	hdr = skb_vnet_hdr(skb);
> >  
> > --
> > 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang
wrote:
> When mergeable buffer were used, we only put the first page buf leave the
rest
> of buffers in the virt queue. This will cause the driver could not get the
> correct head buffer any more. Fix this by dropping the rest of buffers for
this
> packet.
> 
> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> (virtio_net: Defer skb allocation in receive path).
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Michael Dalton <mwdalton at google.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Cc: Shirley Ma <xma at us.ibm.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
Just to clarify my previous comment: it was not about the
idea of adding drop_mergeable_buffer - rather, I think that
adding knowledge about mergeable buffers into page_to_skb creates an
ugly internal API.

Let's move the call to page_to_skb within receive_mergeable instead:
it's also nice that int offset = buf - page_address(page) logic
is not spread around like it was.

Also, it's not nice that we ignore length errors when we drop
packets because of OOM.

So I came up with the following - it seems to work but I didn't
stress test yet.

commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
Author: Michael S. Tsirkin <mst at redhat.com>
Date:   Wed Nov 20 14:41:29 2013 +0200

    virtio_net: fix error handling for mergeable buffers
    
    Eric Dumazet noticed that if we encounter an error
    when processing a mergeable buffer, we don't
    dequeue all of the buffers from this packet,
    the result is almost sure to be loss of networking.
    
    Jason Wang noticed that we also leak a page and that we don't decrement
    the rq buf count, so we won't repost buffers (a resource leak).
    
    Cc: Rusty Russell <rusty at rustcorp.com.au>
    Cc: Michael Dalton <mwdalton at google.com>
    Reported-by: Eric Dumazet <edumazet at google.com>
    Reported-by: Jason Wang <jasowang at redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst at redhat.com>

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 01f4eb5..42f6a1e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct receive_queue
*rq,
 	return skb;
 }
 
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff
*head_skb)
+static struct sk_buff *receive_mergeable(struct net_device *dev,
+					 struct receive_queue *rq,
+					 void *buf,
+					 unsigned int len)
 {
-	struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
+	struct skb_vnet_hdr *hdr = buf;
+	int num_buf = hdr->mhdr.num_buffers;
+	struct page *page = virt_to_head_page(buf);
+	int offset = buf - page_address(page);
+	struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
+					       MAX_PACKET_LEN);
 	struct sk_buff *curr_skb = head_skb;
-	char *buf;
-	struct page *page;
-	int num_buf, len, offset;
 
-	num_buf = hdr->mhdr.num_buffers;
-	while (--num_buf) {
-		int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
+	if (unlikely(!curr_skb))
+		goto err_skb;
+
+	while (--num_buf) {
+		int num_skb_frags;
+
 		buf = virtqueue_get_buf(rq->vq, &len);
 		if (unlikely(!buf)) {
-			pr_debug("%s: rx error: %d buffers missing\n",
-				 head_skb->dev->name, hdr->mhdr.num_buffers);
-			head_skb->dev->stats.rx_length_errors++;
-			return -EINVAL;
+			pr_debug("%s: rx error: %d buffers out of %d missing\n",
+				 dev->name, num_buf, hdr->mhdr.num_buffers);
+			dev->stats.rx_length_errors++;
+			goto err_buf;
 		}
 		if (unlikely(len > MAX_PACKET_LEN)) {
 			pr_debug("%s: rx error: merge buffer too long\n",
-				 head_skb->dev->name);
+				 dev->name);
 			len = MAX_PACKET_LEN;
 		}
+
+		page = virt_to_head_page(buf);
+		--rq->num;
+
+		num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
 		if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
 			struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
-			if (unlikely(!nskb)) {
-				head_skb->dev->stats.rx_dropped++;
-				return -ENOMEM;
-			}
+
+			if (unlikely(!nskb))
+				goto err_skb;
 			if (curr_skb == head_skb)
 				skb_shinfo(curr_skb)->frag_list = nskb;
 			else
 				curr_skb->next = nskb;
-			curr_skb = nskb;
 			head_skb->truesize += nskb->truesize;
+			curr_skb = nskb;
 			num_skb_frags = 0;
 		}
 		if (curr_skb != head_skb) {
@@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 			head_skb->len += len;
 			head_skb->truesize += MAX_PACKET_LEN;
 		}
-		page = virt_to_head_page(buf);
-		offset = buf - (char *)page_address(page);
+		offset = buf - page_address(page);
 		if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
 			put_page(page);
 			skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
@@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq,
struct sk_buff *head_skb)
 					offset, len,
 					MAX_PACKET_LEN);
 		}
+	}
+
+	return head_skb;
+
+err_skb:
+	put_page(page);
+err_buf:
+	dev->stats.rx_dropped++;
+	dev_kfree_skb(head_skb);
+	while (--num_buf) {
+		buf = virtqueue_get_buf(rq->vq, &len);
+		if (unlikely(!buf)) {
+			pr_debug("%s: rx error: %d buffers missing\n",
+				 dev->name, num_buf);
+			dev->stats.rx_length_errors++;
+			break;
+		}
+		page = virt_to_head_page(buf);
+		put_page(page);
 		--rq->num;
 	}
-	return 0;
+	return NULL;
 }
 
 static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void
*buf, unsigned int len)
 		len -= sizeof(struct virtio_net_hdr);
 		skb_trim(skb, len);
 	} else if (vi->mergeable_rx_bufs) {
-		struct page *page = virt_to_head_page(buf);
-		skb = page_to_skb(rq, page,
-				  (char *)buf - (char *)page_address(page),
-				  len, MAX_PACKET_LEN);
-		if (unlikely(!skb)) {
-			dev->stats.rx_dropped++;
-			put_page(page);
+		skb = receive_mergeable(dev, rq, buf, len);
+		if (unlikely(!skb))
 			return;
-		}
-		if (receive_mergeable(rq, skb)) {
-			dev_kfree_skb(skb);
-			return;
-		}
 	} else {
 		page = buf;
 		skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);

----- ???? -----
> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > When mergeable buffer were used, we only put the first page buf leave
the
> > rest
> > of buffers in the virt queue. This will cause the driver could not get
the
> > correct head buffer any more. Fix this by dropping the rest of buffers
for
> > this
> > packet.
> > 
> > The bug was introduced by commit
9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > (virtio_net: Defer skb allocation in receive path).
> > 
> > Cc: Rusty Russell <rusty at rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Cc: Michael Dalton <mwdalton at google.com>
> > Cc: Eric Dumazet <edumazet at google.com>
> > Cc: Shirley Ma <xma at us.ibm.com>
> > Signed-off-by: Jason Wang <jasowang at redhat.com>
> 
> Just to clarify my previous comment: it was not about the
> idea of adding drop_mergeable_buffer - rather, I think that
> adding knowledge about mergeable buffers into page_to_skb creates an
> ugly internal API.
> 
> Let's move the call to page_to_skb within receive_mergeable instead:
> it's also nice that int offset = buf - page_address(page) logic
> is not spread around like it was.
> 
> Also, it's not nice that we ignore length errors when we drop
> packets because of OOM.
> 
> So I came up with the following - it seems to work but I didn't
> stress test yet.
I've no objection on this. But I've rather like my small and direct
patch
to be applied to -net first. It has lower risk and was much more easier to 
be backported to stable trees. Then we can do the re-factor like this in 
net-next. 
> 
> commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
> Author: Michael S. Tsirkin <mst at redhat.com>
> Date:   Wed Nov 20 14:41:29 2013 +0200
> 
>     virtio_net: fix error handling for mergeable buffers
>     
>     Eric Dumazet noticed that if we encounter an error
>     when processing a mergeable buffer, we don't
>     dequeue all of the buffers from this packet,
>     the result is almost sure to be loss of networking.
>     
>     Jason Wang noticed that we also leak a page and that we don't
decrement
>     the rq buf count, so we won't repost buffers (a resource leak).
>     
>     Cc: Rusty Russell <rusty at rustcorp.com.au>
>     Cc: Michael Dalton <mwdalton at google.com>
>     Reported-by: Eric Dumazet <edumazet at google.com>
>     Reported-by: Jason Wang <jasowang at redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 01f4eb5..42f6a1e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct
receive_queue
> *rq,
>  	return skb;
>  }
>  
> -static int receive_mergeable(struct receive_queue *rq, struct sk_buff
> *head_skb)
> +static struct sk_buff *receive_mergeable(struct net_device *dev,
> +					 struct receive_queue *rq,
> +					 void *buf,
> +					 unsigned int len)
>  {
> -	struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
> +	struct skb_vnet_hdr *hdr = buf;
> +	int num_buf = hdr->mhdr.num_buffers;
> +	struct page *page = virt_to_head_page(buf);
> +	int offset = buf - page_address(page);
> +	struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
> +					       MAX_PACKET_LEN);
>  	struct sk_buff *curr_skb = head_skb;
> -	char *buf;
> -	struct page *page;
> -	int num_buf, len, offset;
>  
> -	num_buf = hdr->mhdr.num_buffers;
> -	while (--num_buf) {
> -		int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> +	if (unlikely(!curr_skb))
> +		goto err_skb;
> +
> +	while (--num_buf) {
> +		int num_skb_frags;
> +
>  		buf = virtqueue_get_buf(rq->vq, &len);
>  		if (unlikely(!buf)) {
> -			pr_debug("%s: rx error: %d buffers missing\n",
> -				 head_skb->dev->name, hdr->mhdr.num_buffers);
> -			head_skb->dev->stats.rx_length_errors++;
> -			return -EINVAL;
> +			pr_debug("%s: rx error: %d buffers out of %d missing\n",
> +				 dev->name, num_buf, hdr->mhdr.num_buffers);
> +			dev->stats.rx_length_errors++;
> +			goto err_buf;
>  		}
>  		if (unlikely(len > MAX_PACKET_LEN)) {
>  			pr_debug("%s: rx error: merge buffer too long\n",
> -				 head_skb->dev->name);
> +				 dev->name);
>  			len = MAX_PACKET_LEN;
>  		}
> +
> +		page = virt_to_head_page(buf);
> +		--rq->num;
> +
> +		num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
>  		if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
>  			struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> -			if (unlikely(!nskb)) {
> -				head_skb->dev->stats.rx_dropped++;
> -				return -ENOMEM;
> -			}
> +
> +			if (unlikely(!nskb))
> +				goto err_skb;
>  			if (curr_skb == head_skb)
>  				skb_shinfo(curr_skb)->frag_list = nskb;
>  			else
>  				curr_skb->next = nskb;
> -			curr_skb = nskb;
>  			head_skb->truesize += nskb->truesize;
> +			curr_skb = nskb;
>  			num_skb_frags = 0;
>  		}
>  		if (curr_skb != head_skb) {
> @@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
>  			head_skb->len += len;
>  			head_skb->truesize += MAX_PACKET_LEN;
>  		}
> -		page = virt_to_head_page(buf);
> -		offset = buf - (char *)page_address(page);
> +		offset = buf - page_address(page);
>  		if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
>  			put_page(page);
>  			skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> @@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
>  					offset, len,
>  					MAX_PACKET_LEN);
>  		}
> +	}
> +
> +	return head_skb;
> +
> +err_skb:
> +	put_page(page);
> +err_buf:
> +	dev->stats.rx_dropped++;
> +	dev_kfree_skb(head_skb);
> +	while (--num_buf) {
> +		buf = virtqueue_get_buf(rq->vq, &len);
> +		if (unlikely(!buf)) {
> +			pr_debug("%s: rx error: %d buffers missing\n",
> +				 dev->name, num_buf);
> +			dev->stats.rx_length_errors++;
> +			break;
> +		}
> +		page = virt_to_head_page(buf);
> +		put_page(page);
>  		--rq->num;
>  	}
> -	return 0;
> +	return NULL;
>  }
>  
>  static void receive_buf(struct receive_queue *rq, void *buf, unsigned int
>  len)
> @@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void
> *buf, unsigned int len)
>  		len -= sizeof(struct virtio_net_hdr);
>  		skb_trim(skb, len);
>  	} else if (vi->mergeable_rx_bufs) {
> -		struct page *page = virt_to_head_page(buf);
> -		skb = page_to_skb(rq, page,
> -				  (char *)buf - (char *)page_address(page),
> -				  len, MAX_PACKET_LEN);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			put_page(page);
> +		skb = receive_mergeable(dev, rq, buf, len);
> +		if (unlikely(!skb))
>  			return;
> -		}
> -		if (receive_mergeable(rq, skb)) {
> -			dev_kfree_skb(skb);
> -			return;
> -		}
>  	} else {
>  		page = buf;
>  		skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev"
in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>