Linux Virtualization - Dec 2013 - [PATCH stable 1/2] 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).

Fix both issues.

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

Signed-off-by: Michael S. Tsirkin <mst at 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

Hi Michael, quick question below:

On Wed, Dec 25, 2013 at 6:56 AM, Michael S. Tsirkin <mst at 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

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 at
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

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 at rustcorp.com.au>
> Cc: Michael Dalton <mwdalton at google.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: Jason Wang <jasowang at redhat.com>
> Cc: David S. Miller <davem at davemloft.net>
>
> Signed-off-by: Michael S. Tsirkin <mst at 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);