Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers

[Michael S. Tsirkin]

On Wed, Nov 20, 2013 at 08:54:02AM -0500, Jason Wang wrote:
> 
> 
> ----- ååéä -----
> > 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@xxxxxxxxxxxxxxx>
> > > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > Cc: Michael Dalton <mwdalton@xxxxxxxxxx>
> > > Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> > > Cc: Shirley Ma <xma@xxxxxxxxxx>
> > > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > 
> > 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. 

It makes the interfaces too messy. We are not in code freeze in -net -
only feature freeze, so no reason to make code like spagetty,
and it's only 25 lines changed as compared to 40.
It's not a huge refactoring.

It's just as easy to backport my patch too.
You just drop the goto in the new code path we added.

Let me show you (untested) - your patch is not smaller.

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



commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb
Author: Michael S. Tsirkin <mst@xxxxxxxxxx>
Date:   Wed Nov 20 12:44:14 2013 +0200

    virtio_net: fix resource leak on alloc failure
    
    virtio net got confused, started dropping packets.
    
    Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9fbdfcd..df4b9d0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -297,13 +297,22 @@ 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,
+					 struct page *page,
+					 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 sk_buff *skb = page_to_skb(rq, page, len);
+	int i;
+
+	skb = page_to_skb(rq, page, len);
+
+	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) {
@@ -313,10 +322,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
 		}
 		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 +335,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
 
 		--rq->num;
 	}
-	return 0;
+	return 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 = buf;
+		give_pages(rq, page);
+		--rq->num;
+	}
+	return NULL;
 }
 
 static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -354,17 +381,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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/