* [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers
[not found] <1388064752-15734-1-git-send-email-mst@redhat.com>
@ 2013-12-26 13:32 ` Michael S. Tsirkin
2013-12-27 3:19 ` Jason Wang
2013-12-26 13:32 ` [PATCH stable v2 2/3] virtio-net: make all RX paths handle errors consistently Michael S. Tsirkin
2013-12-26 13:32 ` [PATCH stable v2 3/3] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
2 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2013-12-26 13:32 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.
Fix this issue.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael Dalton <mwdalton@google.com>
Acked-by: Michael Dalton <mwdalton@google.com>
Cc: Eric Dumazet <edumazet@google.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] 5+ messages in thread* Re: [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers
2013-12-26 13:32 ` [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
@ 2013-12-27 3:19 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2013-12-27 3:19 UTC (permalink / raw)
To: Michael S. Tsirkin, netdev
Cc: Michael Dalton, linux-kernel, virtualization, Eric Dumazet,
David Miller
On 12/26/2013 09:32 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.
>
> Fix this issue.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael Dalton <mwdalton@google.com>
> Acked-by: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.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);
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH stable v2 2/3] virtio-net: make all RX paths handle errors consistently
[not found] <1388064752-15734-1-git-send-email-mst@redhat.com>
2013-12-26 13:32 ` [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
@ 2013-12-26 13:32 ` Michael S. Tsirkin
2013-12-27 3:19 ` Jason Wang
2013-12-26 13:32 ` [PATCH stable v2 3/3] virtio_net: don't leak memory or block when too many frags Michael S. Tsirkin
2 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2013-12-26 13:32 UTC (permalink / raw)
To: netdev; +Cc: Michael Dalton, linux-kernel, virtualization, David Miller
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>
Acked-by: Michael Dalton <mwdalton@google.com>
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] 5+ messages in thread* Re: [PATCH stable v2 2/3] virtio-net: make all RX paths handle errors consistently
2013-12-26 13:32 ` [PATCH stable v2 2/3] virtio-net: make all RX paths handle errors consistently Michael S. Tsirkin
@ 2013-12-27 3:19 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2013-12-27 3:19 UTC (permalink / raw)
To: Michael S. Tsirkin, netdev
Cc: linux-kernel, virtualization, David Miller, Michael Dalton
On 12/26/2013 09:32 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>
> Acked-by: Michael Dalton <mwdalton@google.com>
> 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);
>
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH stable v2 3/3] virtio_net: don't leak memory or block when too many frags
[not found] <1388064752-15734-1-git-send-email-mst@redhat.com>
2013-12-26 13:32 ` [PATCH stable v2 1/3] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
2013-12-26 13:32 ` [PATCH stable v2 2/3] virtio-net: make all RX paths handle errors consistently Michael S. Tsirkin
@ 2013-12-26 13:32 ` Michael S. Tsirkin
2 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2013-12-26 13:32 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>
Acked-by: Michael Dalton <mwdalton@google.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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..b8665588 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) {
@@ -365,6 +365,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
err_skb:
give_pages(rq, page);
while (--num_buf) {
+err_frags:
buf = virtqueue_get_buf(rq->vq, &len);
if (unlikely(!buf)) {
pr_debug("%s: rx error: %d buffers missing\n",
--
MST
^ permalink raw reply related [flat|nested] 5+ messages in thread