* virtio_net: Set correct gso->hdr_len
@ 2009-06-04 10:59 Herbert Xu
2009-06-05 4:21 ` Rusty Russell
2009-06-08 7:22 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Herbert Xu @ 2009-06-04 10:59 UTC (permalink / raw)
To: Rusty Russell, David S. Miller, netdev
Hi:
virtio_net: Set correct gso->hdr_len
Through a bug in the tun driver, I noticed that virtio_net is
producing bogus hdr_len values. In particular, it only includes
the IP header in the linear area, and excludes the entire TCP
header. This causes the TCP header to be copied twice for each
packet. (The bug omitted the second copy :)
This patch corrects this.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4d1d479..1c9cedd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -470,7 +470,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
}
if (skb_is_gso(skb)) {
- hdr->hdr_len = skb_transport_header(skb) - skb->data;
+ hdr->hdr_len = skb_headlen(skb);
hdr->gso_size = skb_shinfo(skb)->gso_size;
if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4)
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: virtio_net: Set correct gso->hdr_len
2009-06-04 10:59 virtio_net: Set correct gso->hdr_len Herbert Xu
@ 2009-06-05 4:21 ` Rusty Russell
2009-06-05 4:27 ` Herbert Xu
2009-06-08 7:22 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Rusty Russell @ 2009-06-05 4:21 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
On Thu, 4 Jun 2009 08:29:18 pm Herbert Xu wrote:
> Hi:
>
> virtio_net: Set correct gso->hdr_len
>
> Through a bug in the tun driver, I noticed that virtio_net is
> producing bogus hdr_len values. In particular, it only includes
> the IP header in the linear area, and excludes the entire TCP
> header. This causes the TCP header to be copied twice for each
> packet. (The bug omitted the second copy :)
>
> This patch corrects this.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4d1d479..1c9cedd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -470,7 +470,7 @@ static int xmit_skb(struct virtnet_info *vi, struct
> sk_buff *skb) }
>
> if (skb_is_gso(skb)) {
> - hdr->hdr_len = skb_transport_header(skb) - skb->data;
> + hdr->hdr_len = skb_headlen(skb);
Ouch!
But are we allowed to make the assumption that skb_headlen() ==
skb_transport_header(skb) + sizeof(transport header) ? Or should we be
checking that here?
Also how did this ever work? Why are we getting packets through at all?
Damn, I'm away from my test boxes an I really want to benchmark this fix.
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: virtio_net: Set correct gso->hdr_len
2009-06-05 4:21 ` Rusty Russell
@ 2009-06-05 4:27 ` Herbert Xu
2009-06-05 6:50 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2009-06-05 4:27 UTC (permalink / raw)
To: Rusty Russell; +Cc: David S. Miller, netdev
On Fri, Jun 05, 2009 at 01:51:05PM +0930, Rusty Russell wrote:
>
> But are we allowed to make the assumption that skb_headlen() ==
> skb_transport_header(skb) + sizeof(transport header) ? Or should we be
> checking that here?
We don't care. We just want to preserve whatever the OS gave us.
If it's bogus, the backend will reject it or fix it up.
> Also how did this ever work? Why are we getting packets through at all?
Because the backend fixed it up.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: virtio_net: Set correct gso->hdr_len
2009-06-05 4:27 ` Herbert Xu
@ 2009-06-05 6:50 ` Rusty Russell
2009-06-05 6:53 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Rusty Russell @ 2009-06-05 6:50 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
On Fri, 5 Jun 2009 01:57:09 pm Herbert Xu wrote:
> On Fri, Jun 05, 2009 at 01:51:05PM +0930, Rusty Russell wrote:
> > But are we allowed to make the assumption that skb_headlen() ==
> > skb_transport_header(skb) + sizeof(transport header) ? Or should we be
> > checking that here?
>
> We don't care. We just want to preserve whatever the OS gave us.
> If it's bogus, the backend will reject it or fix it up.
Right, understood now. It really is just a hint as to how much to copy. It
wasn't supposed to be that, as virtio_net.h:
__u16 hdr_len; /* Ethernet + IP + tcp/udp hdrs */
__u16 gso_size; /* Bytes to append to hdr_len per frame */
The host was supposed to be able to de-gso packets together relying on
hdr_len. But that's silly: it can't do TSO without knowing about sequence
numbers at least, and UFO without ip fragmentation offsets. ie. it might as
well calc hdr_len itself.
I'm working on a proper spec, I'll be sure to note this.
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: virtio_net: Set correct gso->hdr_len
2009-06-05 6:50 ` Rusty Russell
@ 2009-06-05 6:53 ` Herbert Xu
0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2009-06-05 6:53 UTC (permalink / raw)
To: Rusty Russell; +Cc: David S. Miller, netdev
On Fri, Jun 05, 2009 at 04:20:02PM +0930, Rusty Russell wrote:
>
> Right, understood now. It really is just a hint as to how much to copy. It
> wasn't supposed to be that, as virtio_net.h:
>
> __u16 hdr_len; /* Ethernet + IP + tcp/udp hdrs */
> __u16 gso_size; /* Bytes to append to hdr_len per frame */
>
> The host was supposed to be able to de-gso packets together relying on
> hdr_len. But that's silly: it can't do TSO without knowing about sequence
> numbers at least, and UFO without ip fragmentation offsets. ie. it might as
> well calc hdr_len itself.
Well I always thought hdr_len was meant to represent what should
be in the linear header :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: virtio_net: Set correct gso->hdr_len
2009-06-04 10:59 virtio_net: Set correct gso->hdr_len Herbert Xu
2009-06-05 4:21 ` Rusty Russell
@ 2009-06-08 7:22 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2009-06-08 7:22 UTC (permalink / raw)
To: herbert; +Cc: rusty, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 4 Jun 2009 20:59:18 +1000
> Hi:
>
> virtio_net: Set correct gso->hdr_len
>
> Through a bug in the tun driver, I noticed that virtio_net is
> producing bogus hdr_len values. In particular, it only includes
> the IP header in the linear area, and excludes the entire TCP
> header. This causes the TCP header to be copied twice for each
> packet. (The bug omitted the second copy :)
>
> This patch corrects this.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-06-08 7:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-04 10:59 virtio_net: Set correct gso->hdr_len Herbert Xu
2009-06-05 4:21 ` Rusty Russell
2009-06-05 4:27 ` Herbert Xu
2009-06-05 6:50 ` Rusty Russell
2009-06-05 6:53 ` Herbert Xu
2009-06-08 7:22 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).