Jason Wang
2013-Nov-20 13:54 UTC
[PATCH RFC] virtio_net: fix error handling for mergeable buffers
----- ???? -----> 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 >
Michael S. Tsirkin
2013-Nov-20 14:20 UTC
[PATCH RFC] virtio_net: fix error handling for mergeable buffers
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 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.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 at redhat.com> commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb Author: Michael S. Tsirkin <mst at redhat.com> 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 at redhat.com> 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);
Jason Wang
2013-Nov-21 03:27 UTC
[PATCH RFC] virtio_net: fix error handling for mergeable buffers
On 11/20/2013 10:20 PM, Michael S. Tsirkin wrote:> 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 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. > It makes the interfaces too messy.Do you mean receive_mergeable() should call page_to_skb()? It has been there several years. And even with your changes, for big and small packets, page_to_skb() were still called directly receive_buf() and the error handling were done there.> 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.The problem is your patch mixes several bugs fixing with re-factoring, only one of the bug fixing was really needed for stable. At least we should make incremental patches on this.> > It's just as easy to backport my patch too. > You just drop the goto in the new code path we added.It may not be so easy for the people who are not familiar with virtio-net and the s/skb->dev/dev/g changes looks irrelevant here.> Let me show you (untested) - your patch is not smaller. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > > commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb > Author: Michael S. Tsirkin <mst at redhat.com> > 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 at redhat.com> > > 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);
Reasonably Related Threads
- [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers
- [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers
- [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
- [PATCH stable 1/2] virtio_net: fix error handling for mergeable buffers
- [PATCH net-next] virtio_net: migrate mergeable rx buffers to page frag allocators