netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH stable 0/2] virtio-net: backport error handling bugfix
@ 2013-12-25 14:56 Michael S. Tsirkin
  2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2013-12-25 14:56 UTC (permalink / raw)
  To: netdev; +Cc: David Miller, Rusty Russell

Error handling for virtio-net mergeable buffers was broken
for a long time. Backport recent bugfix to a stable
kernel.

Please note that it's really a single patch split out in two for ease of
review: if we apply 1/2 only the code is correct but ugly and confusing, so I
think it's best to backport both patches 1/2 and 2/2 same as upstream has them.

Please review and consider for stable.

Michael S. Tsirkin (2):
  virtio_net: fix error handling for mergeable buffers
  virtio-net: make all RX paths handle erors consistently

 drivers/net/virtio_net.c | 103 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 74 insertions(+), 29 deletions(-)

-- 
MST

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
  2013-12-25 14:56 [PATCH stable 0/2] virtio-net: backport error handling bugfix Michael S. Tsirkin
@ 2013-12-25 14:56 ` Michael S. Tsirkin
  2013-12-25 18:33   ` Michael Dalton
  2013-12-26  7:28   ` Jason Wang
  2013-12-25 14:56 ` [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently Michael S. Tsirkin
  2013-12-25 20:13 ` [PATCH stable] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
  2 siblings, 2 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2013-12-25 14:56 UTC (permalink / raw)
  To: netdev
  Cc: Michael Dalton, linux-kernel, virtualization, Eric Dumazet,
	David Miller

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).

Fix both issues.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael Dalton <mwdalton@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: David S. Miller <davem@davemloft.net>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

(cherry picked from commit 8fc3b9e9a229778e5af3aa453c44f1a3857ba769)
---
 drivers/net/virtio_net.c | 66 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9fbdfcd..435076f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -297,26 +297,33 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
 	return skb;
 }
 
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff *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(skb);
-	struct page *page;
-	int num_buf, i, len;
+	struct skb_vnet_hdr *hdr = page_address(buf);
+	int num_buf = hdr->mhdr.num_buffers;
+	struct page *page = buf;
+	struct sk_buff *skb = page_to_skb(rq, page, len);
+	int i;
+
+	if (unlikely(!skb))
+		goto err_skb;
 
-	num_buf = hdr->mhdr.num_buffers;
 	while (--num_buf) {
 		i = skb_shinfo(skb)->nr_frags;
 		if (i >= MAX_SKB_FRAGS) {
 			pr_debug("%s: packet too long\n", skb->dev->name);
 			skb->dev->stats.rx_length_errors++;
-			return -EINVAL;
+			return NULL;
 		}
 		page = virtqueue_get_buf(rq->vq, &len);
 		if (!page) {
-			pr_debug("%s: rx error: %d buffers missing\n",
-				 skb->dev->name, hdr->mhdr.num_buffers);
-			skb->dev->stats.rx_length_errors++;
-			return -EINVAL;
+			pr_debug("%s: rx error: %d buffers %d missing\n",
+				 dev->name, hdr->mhdr.num_buffers, num_buf);
+			dev->stats.rx_length_errors++;
+			goto err_buf;
 		}
 
 		if (len > PAGE_SIZE)
@@ -326,7 +333,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
 
 		--rq->num;
 	}
-	return 0;
+	return skb;
+err_skb:
+	give_pages(rq, page);
+	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 = buf;
+		give_pages(rq, page);
+		--rq->num;
+	}
+err_buf:
+	dev->stats.rx_dropped++;
+	dev_kfree_skb(skb);
+	return NULL;
 }
 
 static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -354,17 +379,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
 		skb_trim(skb, len);
 	} else {
 		page = buf;
-		skb = page_to_skb(rq, page, len);
-		if (unlikely(!skb)) {
-			dev->stats.rx_dropped++;
-			give_pages(rq, page);
-			return;
-		}
-		if (vi->mergeable_rx_bufs)
-			if (receive_mergeable(rq, skb)) {
-				dev_kfree_skb(skb);
+		if (vi->mergeable_rx_bufs) {
+			skb = receive_mergeable(dev, rq, page, len);
+			if (unlikely(!skb))
+				return;
+		} else {
+			skb = page_to_skb(rq, page, len);
+			if (unlikely(!skb)) {
+				dev->stats.rx_dropped++;
+				give_pages(rq, page);
 				return;
 			}
+		}
 	}
 
 	hdr = skb_vnet_hdr(skb);
-- 
MST

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently
  2013-12-25 14:56 [PATCH stable 0/2] virtio-net: backport error handling bugfix Michael S. Tsirkin
  2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
@ 2013-12-25 14:56 ` Michael S. Tsirkin
  2013-12-26  7:28   ` Jason Wang
  2013-12-26  7:31   ` Zhi Yong Wu
  2013-12-25 20:13 ` [PATCH stable] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
  2 siblings, 2 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2013-12-25 14:56 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, David Miller, virtualization

receive mergeable now handles errors internally.
Do same for big and small packet paths, otherwise
the logic is too hard to follow.

Cc: Jason Wang <jasowang@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

(cherry picked from commit f121159d72091f25afb22007c833e60a6845e912)
---
 drivers/net/virtio_net.c | 56 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 435076f..c0ed6d5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -297,6 +297,34 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
 	return skb;
 }
 
+static struct sk_buff *receive_small(void *buf, unsigned int len)
+{
+	struct sk_buff * skb = buf;
+
+	len -= sizeof(struct virtio_net_hdr);
+	skb_trim(skb, len);
+
+	return skb;
+}
+
+static struct sk_buff *receive_big(struct net_device *dev,
+				   struct receive_queue *rq,
+				   void *buf)
+{
+	struct page *page = buf;
+	struct sk_buff *skb = page_to_skb(rq, page, 0);
+
+	if (unlikely(!skb))
+		goto err;
+
+	return skb;
+
+err:
+	dev->stats.rx_dropped++;
+	give_pages(rq, page);
+	return NULL;
+}
+
 static struct sk_buff *receive_mergeable(struct net_device *dev,
 					 struct receive_queue *rq,
 					 void *buf,
@@ -360,7 +388,6 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
 	struct net_device *dev = vi->dev;
 	struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
 	struct sk_buff *skb;
-	struct page *page;
 	struct skb_vnet_hdr *hdr;
 
 	if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
@@ -372,26 +399,15 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
 			dev_kfree_skb(buf);
 		return;
 	}
+	if (vi->mergeable_rx_bufs)
+		skb = receive_mergeable(dev, rq, buf, len);
+	else if (vi->big_packets)
+		skb = receive_big(dev, rq, buf);
+	else
+		skb = receive_small(buf, len);
 
-	if (!vi->mergeable_rx_bufs && !vi->big_packets) {
-		skb = buf;
-		len -= sizeof(struct virtio_net_hdr);
-		skb_trim(skb, len);
-	} else {
-		page = buf;
-		if (vi->mergeable_rx_bufs) {
-			skb = receive_mergeable(dev, rq, page, len);
-			if (unlikely(!skb))
-				return;
-		} else {
-			skb = page_to_skb(rq, page, len);
-			if (unlikely(!skb)) {
-				dev->stats.rx_dropped++;
-				give_pages(rq, page);
-				return;
-			}
-		}
-	}
+	if (unlikely(!skb))
+		return;
 
 	hdr = skb_vnet_hdr(skb);
 
-- 
MST

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
  2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
@ 2013-12-25 18:33   ` Michael Dalton
  2013-12-25 19:19     ` Michael S. Tsirkin
  2013-12-26  7:28   ` Jason Wang
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Dalton @ 2013-12-25 18:33 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: netdev, linux-kernel, lf-virt, Eric Dumazet, David Miller

Hi Michael, quick question below:

On Wed, Dec 25, 2013 at 6:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>                 if (i >= MAX_SKB_FRAGS) {
>                         pr_debug("%s: packet too long\n", skb->dev->name);
>                         skb->dev->stats.rx_length_errors++;
> -                       return -EINVAL;
> +                       return NULL;
>                 }

Should this error handling path free the SKB before returning NULL?
It seems like if we just return NULL we may leak memory.

Best,

Mike

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
  2013-12-25 18:33   ` Michael Dalton
@ 2013-12-25 19:19     ` Michael S. Tsirkin
  2013-12-26  6:35       ` Michael Dalton
  0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2013-12-25 19:19 UTC (permalink / raw)
  To: Michael Dalton; +Cc: netdev, linux-kernel, lf-virt, Eric Dumazet, David Miller

On Wed, Dec 25, 2013 at 10:33:37AM -0800, Michael Dalton wrote:
> Hi Michael, quick question below:
> 
> On Wed, Dec 25, 2013 at 6:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >                 if (i >= MAX_SKB_FRAGS) {
> >                         pr_debug("%s: packet too long\n", skb->dev->name);
> >                         skb->dev->stats.rx_length_errors++;
> > -                       return -EINVAL;
> > +                       return NULL;
> >                 }
> 
> Should this error handling path free the SKB before returning NULL?
> It seems like if we just return NULL we may leak memory.
> 
> Best,
> 
> Mike

It's a device error, but
I agree, if we touch this code anyway there's no reason not to handle
this consistently and do goto toward end of file.
It's not a backport anymore though - this code is gone upstream,
so I'll make it a separate patch I think.

-- 
MST

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH stable] virtio_net: don't leak memory or block when too many frags
  2013-12-25 14:56 [PATCH stable 0/2] virtio-net: backport error handling bugfix Michael S. Tsirkin
  2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
  2013-12-25 14:56 ` [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently Michael S. Tsirkin
@ 2013-12-25 20:13 ` Michael S. Tsirkin
  2013-12-26  6:35   ` Michael Dalton
  2 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2013-12-25 20:13 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, virtualization, David Miller, Michael Dalton

We leak an skb when there are too many frags,
we also stop processing the packet in the middle,
the result is almost sure to be loss of networking.

Reported-by: Michael Dalton <mwdalton@google.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Note: this path is gone upstream, so this patch is
for stable kernel only.
This is on top of series:
	virtio-net: backport error handling bugfix
that I sent previously (and that this is in reply to).

 drivers/net/virtio_net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c0ed6d5..86fe1e7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -344,7 +344,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		if (i >= MAX_SKB_FRAGS) {
 			pr_debug("%s: packet too long\n", skb->dev->name);
 			skb->dev->stats.rx_length_errors++;
-			return NULL;
+			goto err_frags;
 		}
 		page = virtqueue_get_buf(rq->vq, &len);
 		if (!page) {
@@ -364,6 +364,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	return skb;
 err_skb:
 	give_pages(rq, page);
 	while (--num_buf) {
+err_frags:
 		buf = virtqueue_get_buf(rq->vq, &len);
 		if (unlikely(!buf)) {
-- 
MST

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
  2013-12-25 19:19     ` Michael S. Tsirkin
@ 2013-12-26  6:35       ` Michael Dalton
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Dalton @ 2013-12-26  6:35 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: netdev, linux-kernel, lf-virt, Eric Dumazet, David Miller

Acked-by: Michael Dalton <mwdalton@google.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH stable] virtio_net: don't leak memory or block when too many frags
  2013-12-25 20:13 ` [PATCH stable] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
@ 2013-12-26  6:35   ` Michael Dalton
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Dalton @ 2013-12-26  6:35 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, David Miller, linux-kernel, lf-virt

Acked-by: Michael Dalton <mwdalton@google.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
  2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
  2013-12-25 18:33   ` Michael Dalton
@ 2013-12-26  7:28   ` Jason Wang
  1 sibling, 0 replies; 11+ messages in thread
From: Jason Wang @ 2013-12-26  7:28 UTC (permalink / raw)
  To: Michael S. Tsirkin, netdev
  Cc: Michael Dalton, linux-kernel, virtualization, Eric Dumazet,
	David Miller

On 12/25/2013 10:56 PM, Michael S. Tsirkin wrote:
> 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).

This issue does not existed in stable tree.
> Fix both issues.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> (cherry picked from commit 8fc3b9e9a229778e5af3aa453c44f1a3857ba769)
> ---
>  drivers/net/virtio_net.c | 66 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9fbdfcd..435076f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -297,26 +297,33 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
>  	return skb;
>  }
>  
> -static int receive_mergeable(struct receive_queue *rq, struct sk_buff *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(skb);
> -	struct page *page;
> -	int num_buf, i, len;
> +	struct skb_vnet_hdr *hdr = page_address(buf);
> +	int num_buf = hdr->mhdr.num_buffers;
> +	struct page *page = buf;
> +	struct sk_buff *skb = page_to_skb(rq, page, len);
> +	int i;
> +
> +	if (unlikely(!skb))
> +		goto err_skb;
>  
> -	num_buf = hdr->mhdr.num_buffers;
>  	while (--num_buf) {
>  		i = skb_shinfo(skb)->nr_frags;
>  		if (i >= MAX_SKB_FRAGS) {
>  			pr_debug("%s: packet too long\n", skb->dev->name);
>  			skb->dev->stats.rx_length_errors++;
> -			return -EINVAL;
> +			return NULL;
>  		}
>  		page = virtqueue_get_buf(rq->vq, &len);
>  		if (!page) {
> -			pr_debug("%s: rx error: %d buffers missing\n",
> -				 skb->dev->name, hdr->mhdr.num_buffers);
> -			skb->dev->stats.rx_length_errors++;
> -			return -EINVAL;
> +			pr_debug("%s: rx error: %d buffers %d missing\n",
> +				 dev->name, hdr->mhdr.num_buffers, num_buf);
> +			dev->stats.rx_length_errors++;
> +			goto err_buf;
>  		}
>  
>  		if (len > PAGE_SIZE)
> @@ -326,7 +333,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
>  
>  		--rq->num;
>  	}
> -	return 0;
> +	return skb;
> +err_skb:
> +	give_pages(rq, page);
> +	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 = buf;
> +		give_pages(rq, page);
> +		--rq->num;
> +	}
> +err_buf:
> +	dev->stats.rx_dropped++;
> +	dev_kfree_skb(skb);
> +	return NULL;
>  }
>  
>  static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> @@ -354,17 +379,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>  		skb_trim(skb, len);
>  	} else {
>  		page = buf;
> -		skb = page_to_skb(rq, page, len);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			give_pages(rq, page);
> -			return;
> -		}
> -		if (vi->mergeable_rx_bufs)
> -			if (receive_mergeable(rq, skb)) {
> -				dev_kfree_skb(skb);
> +		if (vi->mergeable_rx_bufs) {
> +			skb = receive_mergeable(dev, rq, page, len);
> +			if (unlikely(!skb))
> +				return;
> +		} else {
> +			skb = page_to_skb(rq, page, len);
> +			if (unlikely(!skb)) {
> +				dev->stats.rx_dropped++;
> +				give_pages(rq, page);
>  				return;
>  			}
> +		}
>  	}
>  
>  	hdr = skb_vnet_hdr(skb);

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently
  2013-12-25 14:56 ` [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently Michael S. Tsirkin
@ 2013-12-26  7:28   ` Jason Wang
  2013-12-26  7:31   ` Zhi Yong Wu
  1 sibling, 0 replies; 11+ messages in thread
From: Jason Wang @ 2013-12-26  7:28 UTC (permalink / raw)
  To: Michael S. Tsirkin, netdev; +Cc: linux-kernel, David Miller, virtualization

On 12/25/2013 10:56 PM, Michael S. Tsirkin wrote:
> receive mergeable now handles errors internally.
> Do same for big and small packet paths, otherwise
> the logic is too hard to follow.
>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> (cherry picked from commit f121159d72091f25afb22007c833e60a6845e912)
> ---
>  drivers/net/virtio_net.c | 56 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 435076f..c0ed6d5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -297,6 +297,34 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
>  	return skb;
>  }
>  
> +static struct sk_buff *receive_small(void *buf, unsigned int len)
> +{
> +	struct sk_buff * skb = buf;
> +
> +	len -= sizeof(struct virtio_net_hdr);
> +	skb_trim(skb, len);
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *receive_big(struct net_device *dev,
> +				   struct receive_queue *rq,
> +				   void *buf)
> +{
> +	struct page *page = buf;
> +	struct sk_buff *skb = page_to_skb(rq, page, 0);
> +
> +	if (unlikely(!skb))
> +		goto err;
> +
> +	return skb;
> +
> +err:
> +	dev->stats.rx_dropped++;
> +	give_pages(rq, page);
> +	return NULL;
> +}
> +
>  static struct sk_buff *receive_mergeable(struct net_device *dev,
>  					 struct receive_queue *rq,
>  					 void *buf,
> @@ -360,7 +388,6 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>  	struct net_device *dev = vi->dev;
>  	struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>  	struct sk_buff *skb;
> -	struct page *page;
>  	struct skb_vnet_hdr *hdr;
>  
>  	if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> @@ -372,26 +399,15 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>  			dev_kfree_skb(buf);
>  		return;
>  	}
> +	if (vi->mergeable_rx_bufs)
> +		skb = receive_mergeable(dev, rq, buf, len);
> +	else if (vi->big_packets)
> +		skb = receive_big(dev, rq, buf);
> +	else
> +		skb = receive_small(buf, len);
>  
> -	if (!vi->mergeable_rx_bufs && !vi->big_packets) {
> -		skb = buf;
> -		len -= sizeof(struct virtio_net_hdr);
> -		skb_trim(skb, len);
> -	} else {
> -		page = buf;
> -		if (vi->mergeable_rx_bufs) {
> -			skb = receive_mergeable(dev, rq, page, len);
> -			if (unlikely(!skb))
> -				return;
> -		} else {
> -			skb = page_to_skb(rq, page, len);
> -			if (unlikely(!skb)) {
> -				dev->stats.rx_dropped++;
> -				give_pages(rq, page);
> -				return;
> -			}
> -		}
> -	}
> +	if (unlikely(!skb))
> +		return;
>  
>  	hdr = skb_vnet_hdr(skb);
>  
Ack-ed by: Jason Wang <jasowang@redhat.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently
  2013-12-25 14:56 ` [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently Michael S. Tsirkin
  2013-12-26  7:28   ` Jason Wang
@ 2013-12-26  7:31   ` Zhi Yong Wu
  1 sibling, 0 replies; 11+ messages in thread
From: Zhi Yong Wu @ 2013-12-26  7:31 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Linux Netdev List, lf-virt, linux-kernel mlist, David Miller

typo in the subject....

s/erors/errors/

On Wed, Dec 25, 2013 at 10:56 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> receive mergeable now handles errors internally.
> Do same for big and small packet paths, otherwise
> the logic is too hard to follow.
>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> (cherry picked from commit f121159d72091f25afb22007c833e60a6845e912)
> ---
>  drivers/net/virtio_net.c | 56 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 435076f..c0ed6d5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -297,6 +297,34 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
>         return skb;
>  }
>
> +static struct sk_buff *receive_small(void *buf, unsigned int len)
> +{
> +       struct sk_buff * skb = buf;
> +
> +       len -= sizeof(struct virtio_net_hdr);
> +       skb_trim(skb, len);
> +
> +       return skb;
> +}
> +
> +static struct sk_buff *receive_big(struct net_device *dev,
> +                                  struct receive_queue *rq,
> +                                  void *buf)
> +{
> +       struct page *page = buf;
> +       struct sk_buff *skb = page_to_skb(rq, page, 0);
> +
> +       if (unlikely(!skb))
> +               goto err;
> +
> +       return skb;
> +
> +err:
> +       dev->stats.rx_dropped++;
> +       give_pages(rq, page);
> +       return NULL;
> +}
> +
>  static struct sk_buff *receive_mergeable(struct net_device *dev,
>                                          struct receive_queue *rq,
>                                          void *buf,
> @@ -360,7 +388,6 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>         struct net_device *dev = vi->dev;
>         struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>         struct sk_buff *skb;
> -       struct page *page;
>         struct skb_vnet_hdr *hdr;
>
>         if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> @@ -372,26 +399,15 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>                         dev_kfree_skb(buf);
>                 return;
>         }
> +       if (vi->mergeable_rx_bufs)
> +               skb = receive_mergeable(dev, rq, buf, len);
> +       else if (vi->big_packets)
> +               skb = receive_big(dev, rq, buf);
> +       else
> +               skb = receive_small(buf, len);
>
> -       if (!vi->mergeable_rx_bufs && !vi->big_packets) {
> -               skb = buf;
> -               len -= sizeof(struct virtio_net_hdr);
> -               skb_trim(skb, len);
> -       } else {
> -               page = buf;
> -               if (vi->mergeable_rx_bufs) {
> -                       skb = receive_mergeable(dev, rq, page, len);
> -                       if (unlikely(!skb))
> -                               return;
> -               } else {
> -                       skb = page_to_skb(rq, page, len);
> -                       if (unlikely(!skb)) {
> -                               dev->stats.rx_dropped++;
> -                               give_pages(rq, page);
> -                               return;
> -                       }
> -               }
> -       }
> +       if (unlikely(!skb))
> +               return;
>
>         hdr = skb_vnet_hdr(skb);
>
> --
> MST
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization



-- 
Regards,

Zhi Yong Wu

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-12-26  7:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-25 14:56 [PATCH stable 0/2] virtio-net: backport error handling bugfix Michael S. Tsirkin
2013-12-25 14:56 ` [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
2013-12-25 18:33   ` Michael Dalton
2013-12-25 19:19     ` Michael S. Tsirkin
2013-12-26  6:35       ` Michael Dalton
2013-12-26  7:28   ` Jason Wang
2013-12-25 14:56 ` [PATCH stable 2/2] virtio-net: make all RX paths handle erors consistently Michael S. Tsirkin
2013-12-26  7:28   ` Jason Wang
2013-12-26  7:31   ` Zhi Yong Wu
2013-12-25 20:13 ` [PATCH stable] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
2013-12-26  6:35   ` Michael Dalton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).