* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
@ 2013-12-06 0:09 ` Michael Dalton
2013-12-06 4:20 ` Jason Wang
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Michael Dalton @ 2013-12-06 0:09 UTC (permalink / raw)
To: David S. Miller
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, lf-virt, Eric Dumazet,
Michael Dalton
Hi,
A quick note on this patch: I have confirmed that without this
patch a kernel crash occurs if we force a 'packet too short' error
sufficiently many times. This patch eliminates the kernel crash.
Since this crash would be triggered by a hypervisor bug, I made a
small change not reflected in the above patch to make the crash easier
to reproduce for testing purposes. I treated 1 out of every 128 packets
with len < MERGE_BUFFER_LEN as 'too short'. With this change in
place, just running netperf will cause the sender to crash very quickly
(the receiver will transmit pure data ACKs that meet the drop criteria).
If anyone would like to reproduce the crash using the above setup,
I added an unsigned int num_packets field to struct receive_queue and
changed the if condition for the packet too short check in receive_buf()
from:
if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
to:
if (unlikely((len < sizeof(struct virtio_net_hdr) + ETH_HLEN) ||
(len < MERGE_BUFFER_LEN &&
((++rq->num_packets & 127) == 0)))) {
Best,
Mike
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
2013-12-06 0:09 ` Michael Dalton
@ 2013-12-06 4:20 ` Jason Wang
2013-12-06 5:09 ` Andrew Vagin
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2013-12-06 4:20 UTC (permalink / raw)
To: Michael Dalton, David S. Miller
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, virtualization,
Eric Dumazet
On 12/06/2013 05:14 AM, Michael Dalton wrote:
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
> ---
> drivers/net/virtio_net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 916241d..6a4665c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -426,10 +426,10 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> dev->stats.rx_length_errors++;
> - if (vi->big_packets)
> - give_pages(rq, buf);
> - else if (vi->mergeable_rx_bufs)
> + if (vi->mergeable_rx_bufs)
> put_page(virt_to_head_page(buf));
> + else if (vi->big_packets)
> + give_pages(rq, buf);
> else
> dev_kfree_skb(buf);
> return;
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
2013-12-06 0:09 ` Michael Dalton
2013-12-06 4:20 ` Jason Wang
@ 2013-12-06 5:09 ` Andrew Vagin
2013-12-06 5:09 ` Andrew Vagin
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Andrew Vagin @ 2013-12-06 5:09 UTC (permalink / raw)
To: Michael Dalton
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, virtualization,
Eric Dumazet, David S. Miller
On Thu, Dec 05, 2013 at 01:14:05PM -0800, Michael Dalton wrote:
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Acked-by: Andrew Vagin <avagin@openvz.org>
> ---
> drivers/net/virtio_net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 916241d..6a4665c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -426,10 +426,10 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> dev->stats.rx_length_errors++;
> - if (vi->big_packets)
> - give_pages(rq, buf);
> - else if (vi->mergeable_rx_bufs)
> + if (vi->mergeable_rx_bufs)
> put_page(virt_to_head_page(buf));
> + else if (vi->big_packets)
> + give_pages(rq, buf);
> else
> dev_kfree_skb(buf);
> return;
> --
> 1.8.5.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
` (2 preceding siblings ...)
2013-12-06 5:09 ` Andrew Vagin
@ 2013-12-06 5:09 ` Andrew Vagin
2013-12-06 21:32 ` David Miller
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Andrew Vagin @ 2013-12-06 5:09 UTC (permalink / raw)
To: Michael Dalton
Cc: David S. Miller, netdev, Eric Dumazet, Rusty Russell,
Michael S. Tsirkin, Jason Wang, Andrey Vagin, virtualization
On Thu, Dec 05, 2013 at 01:14:05PM -0800, Michael Dalton wrote:
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Acked-by: Andrew Vagin <avagin@openvz.org>
> ---
> drivers/net/virtio_net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 916241d..6a4665c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -426,10 +426,10 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> dev->stats.rx_length_errors++;
> - if (vi->big_packets)
> - give_pages(rq, buf);
> - else if (vi->mergeable_rx_bufs)
> + if (vi->mergeable_rx_bufs)
> put_page(virt_to_head_page(buf));
> + else if (vi->big_packets)
> + give_pages(rq, buf);
> else
> dev_kfree_skb(buf);
> return;
> --
> 1.8.5.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
` (3 preceding siblings ...)
2013-12-06 5:09 ` Andrew Vagin
@ 2013-12-06 21:32 ` David Miller
2013-12-06 23:32 ` Michael Dalton
2013-12-06 21:32 ` David Miller
2013-12-07 0:36 ` Rusty Russell
6 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2013-12-06 21:32 UTC (permalink / raw)
To: mwdalton; +Cc: netdev, edumazet, rusty, mst, jasowang, avagin, virtualization
From: Michael Dalton <mwdalton@google.com>
Date: Thu, 5 Dec 2013 13:14:05 -0800
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Applied, is this needed for -stable?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-06 21:32 ` David Miller
@ 2013-12-06 23:32 ` Michael Dalton
0 siblings, 0 replies; 10+ messages in thread
From: Michael Dalton @ 2013-12-06 23:32 UTC (permalink / raw)
To: David Miller
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, lf-virt, Eric Dumazet
Hi David,
This patch fixes a bug introduced by 2613af0ed18a ("virtio_net:
migrate mergeable rx buffers to page frag allocators"). The bug
is present in both net-next and net. Thanks
Best,
Mike
On Fri, Dec 6, 2013 at 1:32 PM, David Miller <davem@davemloft.net> wrote:
> From: Michael Dalton <mwdalton@google.com>
> Date: Thu, 5 Dec 2013 13:14:05 -0800
>
>> When a packet with invalid length arrives, ensure that the packet
>> is freed correctly if mergeable packet buffers and big packets
>> (GUEST_TSO4) are both enabled.
>>
>> Signed-off-by: Michael Dalton <mwdalton@google.com>
>
> Applied, is this needed for -stable?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
` (4 preceding siblings ...)
2013-12-06 21:32 ` David Miller
@ 2013-12-06 21:32 ` David Miller
2013-12-07 0:36 ` Rusty Russell
6 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2013-12-06 21:32 UTC (permalink / raw)
To: mwdalton; +Cc: avagin, mst, netdev, virtualization, edumazet
From: Michael Dalton <mwdalton@google.com>
Date: Thu, 5 Dec 2013 13:14:05 -0800
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Applied, is this needed for -stable?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
2013-12-05 21:14 [PATCH v2] virtio-net: free bufs correctly on invalid packet length Michael Dalton
@ 2013-12-07 0:36 ` Rusty Russell
2013-12-06 4:20 ` Jason Wang
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Rusty Russell @ 2013-12-07 0:36 UTC (permalink / raw)
To: David S. Miller
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, virtualization,
Eric Dumazet, Michael Dalton
Michael Dalton <mwdalton@google.com> writes:
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks!
> ---
> drivers/net/virtio_net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 916241d..6a4665c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -426,10 +426,10 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> dev->stats.rx_length_errors++;
> - if (vi->big_packets)
> - give_pages(rq, buf);
> - else if (vi->mergeable_rx_bufs)
> + if (vi->mergeable_rx_bufs)
> put_page(virt_to_head_page(buf));
> + else if (vi->big_packets)
> + give_pages(rq, buf);
> else
> dev_kfree_skb(buf);
> return;
> --
> 1.8.5.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] virtio-net: free bufs correctly on invalid packet length
@ 2013-12-07 0:36 ` Rusty Russell
0 siblings, 0 replies; 10+ messages in thread
From: Rusty Russell @ 2013-12-07 0:36 UTC (permalink / raw)
To: Michael Dalton, David S. Miller
Cc: Andrey Vagin, Michael S. Tsirkin, netdev, virtualization,
Eric Dumazet, Michael Dalton
Michael Dalton <mwdalton@google.com> writes:
> When a packet with invalid length arrives, ensure that the packet
> is freed correctly if mergeable packet buffers and big packets
> (GUEST_TSO4) are both enabled.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks!
> ---
> drivers/net/virtio_net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 916241d..6a4665c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -426,10 +426,10 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
> pr_debug("%s: short packet %i\n", dev->name, len);
> dev->stats.rx_length_errors++;
> - if (vi->big_packets)
> - give_pages(rq, buf);
> - else if (vi->mergeable_rx_bufs)
> + if (vi->mergeable_rx_bufs)
> put_page(virt_to_head_page(buf));
> + else if (vi->big_packets)
> + give_pages(rq, buf);
> else
> dev_kfree_skb(buf);
> return;
> --
> 1.8.5.1
^ permalink raw reply [flat|nested] 10+ messages in thread