diff mbox

ipv6: sit: update mtu check to take care of gso packets

Message ID 1387204283.19078.240.camel@edumazet-glaptop2.roam.corp.google.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Dec. 16, 2013, 2:31 p.m. UTC
From: Eric Dumazet <edumazet@google.com>

While testing my changes for TSO support in SIT devices,
I was using sit0 tunnel which appears to include nopmtudisc flag.

But using :

ip tun add sittun mode sit remote $REMOTE_IPV4 local $LOCAL_IPV4 \
   dev $IFACE

We get a tunnel which rejects too long packets because of the mtu check
which is not yet GSO aware.

erd:~# ip tunnel
sittun: ipv6/ip  remote 10.246.17.84  local 10.246.17.83  ttl inherit  6rd-prefix 2002::/16 
sit0: ipv6/ip  remote any  local any  ttl 64  nopmtudisc 6rd-prefix 2002::/16 

This patch is based on an excellent report from
Michal Shmidt.

In the future, we probably want to extend the MTU check to do the
right thing for GSO packets...

Fixes: ("61c1db7fae21 ipv6: sit: add GSO/TSO support")
Reported-by: Michal Schmidt <mschmidt@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/sit.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
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

Comments

Michal Schmidt Dec. 17, 2013, 2:22 p.m. UTC | #1
On 12/16/2013 03:31 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> While testing my changes for TSO support in SIT devices,
> I was using sit0 tunnel which appears to include nopmtudisc flag.
> 
> But using :
> 
> ip tun add sittun mode sit remote $REMOTE_IPV4 local $LOCAL_IPV4 \
>    dev $IFACE
> 
> We get a tunnel which rejects too long packets because of the mtu check
> which is not yet GSO aware.
> 
> erd:~# ip tunnel
> sittun: ipv6/ip  remote 10.246.17.84  local 10.246.17.83  ttl inherit  6rd-prefix 2002::/16 
> sit0: ipv6/ip  remote any  local any  ttl 64  nopmtudisc 6rd-prefix 2002::/16 
> 
> This patch is based on an excellent report from
> Michal Shmidt.
> 
> In the future, we probably want to extend the MTU check to do the
> right thing for GSO packets...
> 
> Fixes: ("61c1db7fae21 ipv6: sit: add GSO/TSO support")
> Reported-by: Michal Schmidt <mschmidt@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Tested-by: Michal Schmidt <mschmidt@redhat.com>

It works fine now. Thanks.
Michal

--
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
Eric Dumazet Dec. 17, 2013, 2:48 p.m. UTC | #2
On Tue, 2013-12-17 at 15:22 +0100, Michal Schmidt wrote:

> Tested-by: Michal Schmidt <mschmidt@redhat.com>
> 
> It works fine now. Thanks.
> Michal

Thanks a lot Michal for your very nice bug report !


--
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
Hannes Frederic Sowa Dec. 17, 2013, 8:19 p.m. UTC | #3
Hi Eric!

On Mon, Dec 16, 2013 at 06:31:23AM -0800, Eric Dumazet wrote:
> In the future, we probably want to extend the MTU check to do the
> right thing for GSO packets...

How could that look like?

Greetings,

  Hannes

--
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
Eric Dumazet Dec. 17, 2013, 9:34 p.m. UTC | #4
On Tue, 2013-12-17 at 21:19 +0100, Hannes Frederic Sowa wrote:
> Hi Eric!
> 
> On Mon, Dec 16, 2013 at 06:31:23AM -0800, Eric Dumazet wrote:
> > In the future, we probably want to extend the MTU check to do the
> > right thing for GSO packets...
> 
> How could that look like?

I think I already posted a draft for this.

Idea would be to have a helper to compute the mtu of each segment :

static inline unsigned int gso_size_with_headers(const struct sk_buff *skb)
{
       unsigned int hdrlen = skb_transport_header(skb) - skb_network_header(skb);

       if (skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))
               hdrlen += tcp_hdrlen(skb);
       else
               hdrlen += 8; // sizeof(struct udphdr)

       return skb_shinfo(skb)->gso_size + hdrlen;
}

...

       len = skb_is_gso(skb) ? gso_size_with_headers(skb) : skb->len;

       if (unlikely(len > dst_mtu(&rt->dst) && ...



--
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
David Miller Dec. 18, 2013, 10:57 p.m. UTC | #5
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Dec 2013 06:31:23 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> While testing my changes for TSO support in SIT devices,
> I was using sit0 tunnel which appears to include nopmtudisc flag.
> 
> But using :
> 
> ip tun add sittun mode sit remote $REMOTE_IPV4 local $LOCAL_IPV4 \
>    dev $IFACE
> 
> We get a tunnel which rejects too long packets because of the mtu check
> which is not yet GSO aware.
> 
> erd:~# ip tunnel
> sittun: ipv6/ip  remote 10.246.17.84  local 10.246.17.83  ttl inherit  6rd-prefix 2002::/16 
> sit0: ipv6/ip  remote any  local any  ttl 64  nopmtudisc 6rd-prefix 2002::/16 
> 
> This patch is based on an excellent report from
> Michal Shmidt.
> 
> In the future, we probably want to extend the MTU check to do the
> right thing for GSO packets...
> 
> Fixes: ("61c1db7fae21 ipv6: sit: add GSO/TSO support")
> Reported-by: Michal Schmidt <mschmidt@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.
--
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 mbox

Patch

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 366fbba3359a..a710fdec42d3 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -924,7 +924,7 @@  static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 		if (tunnel->parms.iph.daddr && skb_dst(skb))
 			skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
 
-		if (skb->len > mtu) {
+		if (skb->len > mtu && !skb_is_gso(skb)) {
 			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 			ip_rt_put(rt);
 			goto tx_error;