| Message ID | 1611805733-25072-1-git-send-email-vfedorenko@novek.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ip_tunnel: fix mtu calculation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: willemb@google.com yoshfuji@linux-ipv6.org xie.he.0141@gmail.com davem@davemloft.net dsahern@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 83 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Jan 27, 2021 at 11:14 PM Vadim Fedorenko <vfedorenko@novek.ru> wrote: > > dev->hard_header_len for tunnel interface is set only when header_ops > are set too and already contains full overhead of any tunnel encapsulation. > That's why there is not need to use this overhead twice in mtu calc. > > Fixes: fdafed459998 ("ip_gre: set dev->hard_header_len and dev->needed_headroom properly") > Reported-by: Slava Bacherikov <mail@slava.cc> > Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> > --- > net/ipv4/ip_tunnel.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c > index 64594aa..ad78825 100644 > --- a/net/ipv4/ip_tunnel.c > +++ b/net/ipv4/ip_tunnel.c > @@ -317,7 +317,7 @@ static int ip_tunnel_bind_dev(struct net_device *dev) > } > > dev->needed_headroom = t_hlen + hlen; > - mtu -= (dev->hard_header_len + t_hlen); > + mtu -= dev->hard_header_len ? : t_hlen; Safety of this change also depends on whether any other ip tunnels might have non-zero hard_header_len. I haven't fully checked yet, but at first scan I only see one other instance of header_ops, and that ip_tunnel_header_ops does not have a create implementation. > > if (mtu < IPV4_MIN_MTU) > mtu = IPV4_MIN_MTU; > @@ -347,7 +347,7 @@ static struct ip_tunnel *ip_tunnel_create(struct net *net, > nt = netdev_priv(dev); > t_hlen = nt->hlen + sizeof(struct iphdr); > dev->min_mtu = ETH_MIN_MTU; > - dev->max_mtu = IP_MAX_MTU - dev->hard_header_len - t_hlen; > + dev->max_mtu = IP_MAX_MTU - dev->hard_header_len ? : t_hlen; here and elsewhere: subtraction takes precedence over ternary conditional, so (IP_MAX_MTU - ..) always true.
On 28.01.2021 21:48, Willem de Bruijn wrote: > On Wed, Jan 27, 2021 at 11:14 PM Vadim Fedorenko <vfedorenko@novek.ru> wrote: >> >> dev->hard_header_len for tunnel interface is set only when header_ops >> are set too and already contains full overhead of any tunnel encapsulation. >> That's why there is not need to use this overhead twice in mtu calc. >> >> Fixes: fdafed459998 ("ip_gre: set dev->hard_header_len and dev->needed_headroom properly") >> Reported-by: Slava Bacherikov <mail@slava.cc> >> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> >> --- >> net/ipv4/ip_tunnel.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c >> index 64594aa..ad78825 100644 >> --- a/net/ipv4/ip_tunnel.c >> +++ b/net/ipv4/ip_tunnel.c >> @@ -317,7 +317,7 @@ static int ip_tunnel_bind_dev(struct net_device *dev) >> } >> >> dev->needed_headroom = t_hlen + hlen; >> - mtu -= (dev->hard_header_len + t_hlen); >> + mtu -= dev->hard_header_len ? : t_hlen; > > Safety of this change also depends on whether any other ip tunnels > might have non-zero hard_header_len. Yes, sure. > I haven't fully checked yet, but at first scan I only see one other > instance of header_ops, and that ip_tunnel_header_ops does not have a > create implementation. Yes. The calls to ip_tunnel_setup are in ip_gre.c, ip_tunnel.c, ip_vti.c and ipip.c. All of them except of ip_gre use ip_tunnel_header_ops which doesn't have create implementation and have hard_header_len set to 0. > >> >> if (mtu < IPV4_MIN_MTU) >> mtu = IPV4_MIN_MTU; >> @@ -347,7 +347,7 @@ static struct ip_tunnel *ip_tunnel_create(struct net *net, >> nt = netdev_priv(dev); >> t_hlen = nt->hlen + sizeof(struct iphdr); >> dev->min_mtu = ETH_MIN_MTU; >> - dev->max_mtu = IP_MAX_MTU - dev->hard_header_len - t_hlen; >> + dev->max_mtu = IP_MAX_MTU - dev->hard_header_len ? : t_hlen; > > here and elsewhere: subtraction takes precedence over ternary > conditional, so (IP_MAX_MTU - ..) always true. Oh, sure, my bad. Will fix it in v2, thanks!
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c index 64594aa..ad78825 100644 --- a/net/ipv4/ip_tunnel.c +++ b/net/ipv4/ip_tunnel.c @@ -317,7 +317,7 @@ static int ip_tunnel_bind_dev(struct net_device *dev) } dev->needed_headroom = t_hlen + hlen; - mtu -= (dev->hard_header_len + t_hlen); + mtu -= dev->hard_header_len ? : t_hlen; if (mtu < IPV4_MIN_MTU) mtu = IPV4_MIN_MTU; @@ -347,7 +347,7 @@ static struct ip_tunnel *ip_tunnel_create(struct net *net, nt = netdev_priv(dev); t_hlen = nt->hlen + sizeof(struct iphdr); dev->min_mtu = ETH_MIN_MTU; - dev->max_mtu = IP_MAX_MTU - dev->hard_header_len - t_hlen; + dev->max_mtu = IP_MAX_MTU - dev->hard_header_len ? : t_hlen; ip_tunnel_add(itn, nt); return nt; @@ -488,11 +488,11 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb, int mtu; tunnel_hlen = md ? tunnel_hlen : tunnel->hlen; - pkt_size = skb->len - tunnel_hlen - dev->hard_header_len; + pkt_size = skb->len - dev->hard_header_len ? : tunnel_hlen; if (df) - mtu = dst_mtu(&rt->dst) - dev->hard_header_len - - sizeof(struct iphdr) - tunnel_hlen; + mtu = dst_mtu(&rt->dst) - dev->hard_header_len ? : + (sizeof(struct iphdr) + tunnel_hlen); else mtu = skb_valid_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu; @@ -972,7 +972,7 @@ int __ip_tunnel_change_mtu(struct net_device *dev, int new_mtu, bool strict) { struct ip_tunnel *tunnel = netdev_priv(dev); int t_hlen = tunnel->hlen + sizeof(struct iphdr); - int max_mtu = IP_MAX_MTU - dev->hard_header_len - t_hlen; + int max_mtu = IP_MAX_MTU - dev->hard_header_len ? : t_hlen; if (new_mtu < ETH_MIN_MTU) return -EINVAL; @@ -1149,10 +1149,10 @@ int ip_tunnel_newlink(struct net_device *dev, struct nlattr *tb[], mtu = ip_tunnel_bind_dev(dev); if (tb[IFLA_MTU]) { - unsigned int max = IP_MAX_MTU - dev->hard_header_len - nt->hlen; + unsigned int max = IP_MAX_MTU - dev->hard_header_len ? : + (nt->hlen + sizeof(struct iphdr)); - mtu = clamp(dev->mtu, (unsigned int)ETH_MIN_MTU, - (unsigned int)(max - sizeof(struct iphdr))); + mtu = clamp(dev->mtu, (unsigned int)ETH_MIN_MTU, max); } err = dev_set_mtu(dev, mtu);
dev->hard_header_len for tunnel interface is set only when header_ops are set too and already contains full overhead of any tunnel encapsulation. That's why there is not need to use this overhead twice in mtu calc. Fixes: fdafed459998 ("ip_gre: set dev->hard_header_len and dev->needed_headroom properly") Reported-by: Slava Bacherikov <mail@slava.cc> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> --- net/ipv4/ip_tunnel.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)