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@xxxxxxxxxxxxxxx>
> Cc: Michael Dalton <mwdalton@xxxxxxxxxx>
> Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: Jason Wang <jasowang@xxxxxxxxxx>
> Cc: Jason Wang <jasowang@xxxxxxxxxx>
> Cc: David S. Miller <davem@xxxxxxxxxxxxx>
>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> (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);
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
