| Message ID | 20090604105917.GA28273@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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> Cheers,