| Message ID | 1380880333-3546-1-git-send-email-ou.ghorbel@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Oct 04, 2013 at 10:52:13AM +0100, Oussama Ghorbel wrote: > Unlike ipv4, the struct member hlen holds the length of the GRE and ipv6 > headers. This length is also counted in dev->hard_header_len. > Perhaps, it's more clean to modify the hlen to count only the GRE header > without ipv6 header as the variable name suggest, but the simple way to fix > this without regression risk is simply modify the calculation of the limit > in ip6gre_tunnel_change_mtu function. > Verified in kernel version v3.11. > > Signed-off-by: Oussama Ghorbel <ou.ghorbel@gmail.com> > --- > net/ipv6/ip6_gre.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > index 90747f1..41487ab 100644 > --- a/net/ipv6/ip6_gre.c > +++ b/net/ipv6/ip6_gre.c > @@ -1175,9 +1175,8 @@ done: > > static int ip6gre_tunnel_change_mtu(struct net_device *dev, int new_mtu) > { > - struct ip6_tnl *tunnel = netdev_priv(dev); > if (new_mtu < 68 || > - new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) > + new_mtu > 0xFFF8 - dev->hard_header_len) > return -EINVAL; > dev->mtu = new_mtu; > return 0; Hmmm... dev->hard_header_len is initialized to LL_MAX_HEADER + sizeof(struct ipv6hdr) + 4 but won't include the additional head space needed for GRE_SEQ, GRE_KEY etc. if at time of tunnel creation the routing table did not had a good guess for the outgoing device. To make this correct we would have to refactor the usage of the variables a bit as is done in ipv4/ip_tunnel.c. The safest thing would be to leave this check as-is currently although we exclude some allowed mtus. Perhaps you want to take a look how to achieve that? ;) 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
On Sat, Oct 5, 2013 at 3:06 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Fri, Oct 04, 2013 at 10:52:13AM +0100, Oussama Ghorbel wrote: >> Unlike ipv4, the struct member hlen holds the length of the GRE and ipv6 >> headers. This length is also counted in dev->hard_header_len. >> Perhaps, it's more clean to modify the hlen to count only the GRE header >> without ipv6 header as the variable name suggest, but the simple way to fix >> this without regression risk is simply modify the calculation of the limit >> in ip6gre_tunnel_change_mtu function. >> Verified in kernel version v3.11. >> >> Signed-off-by: Oussama Ghorbel <ou.ghorbel@gmail.com> >> --- >> net/ipv6/ip6_gre.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c >> index 90747f1..41487ab 100644 >> --- a/net/ipv6/ip6_gre.c >> +++ b/net/ipv6/ip6_gre.c >> @@ -1175,9 +1175,8 @@ done: >> >> static int ip6gre_tunnel_change_mtu(struct net_device *dev, int new_mtu) >> { >> - struct ip6_tnl *tunnel = netdev_priv(dev); >> if (new_mtu < 68 || >> - new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) >> + new_mtu > 0xFFF8 - dev->hard_header_len) >> return -EINVAL; >> dev->mtu = new_mtu; >> return 0; > > Hmmm... > > dev->hard_header_len is initialized to LL_MAX_HEADER + sizeof(struct ipv6hdr) > + 4 but won't include the additional head space needed for GRE_SEQ, GRE_KEY > etc. if at time of tunnel creation the routing table did not had a good guess > for the outgoing device. > This hard_header_len initialization that you have shown above is taken from ip6gre_tunnel_setup, however this same variable seems to be reinitialized in ip6gre_tnl_link_config() which are called from ip6gre_newlink() The initialization in ip6gre_tnl_link_config is done as the following: static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu) { ... int addend = sizeof(struct ipv6hdr) + 4; ... /* Precalculate GRE options length */ if (t->parms.o_flags&(GRE_CSUM|GRE_KEY|GRE_SEQ)) { if (t->parms.o_flags&GRE_CSUM) addend += 4; if (t->parms.o_flags&GRE_KEY) addend += 4; if (t->parms.o_flags&GRE_SEQ) addend += 4; } ... dev->hard_header_len = rt->dst.dev->hard_header_len + addend; ... t->hlen = addend; .. } Unless they are other reasons, the hard_header_len is taken into account the GRE_KEY, GRE_SEQ .. > To make this correct we would have to refactor the usage of the variables a > bit as is done in ipv4/ip_tunnel.c. The safest thing would be to leave this > check as-is currently although we exclude some allowed mtus. > > Perhaps you want to take a look how to achieve that? ;) > Why not, consistency is good ... > Greetings, > > Hannes > Thanks, Oussama -- 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 Sun, Oct 06, 2013 at 03:42:15PM +0100, Oussama Ghorbel wrote: > The initialization in ip6gre_tnl_link_config is done as the following: > static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu) > { > ... > int addend = sizeof(struct ipv6hdr) + 4; > ... > /* Precalculate GRE options length */ > if (t->parms.o_flags&(GRE_CSUM|GRE_KEY|GRE_SEQ)) { > if (t->parms.o_flags&GRE_CSUM) > addend += 4; > if (t->parms.o_flags&GRE_KEY) > addend += 4; > if (t->parms.o_flags&GRE_SEQ) > addend += 4; > } > ... > dev->hard_header_len = rt->dst.dev->hard_header_len + addend; > ... > t->hlen = addend; > .. > } > > Unless they are other reasons, the hard_header_len is taken into > account the GRE_KEY, GRE_SEQ .. But only if a new route is found. The hard_header_len reinitialization is guarded by a (rt == NULL). We may have not found one on boot up. > > To make this correct we would have to refactor the usage of the variables a > > bit as is done in ipv4/ip_tunnel.c. The safest thing would be to leave this > > check as-is currently although we exclude some allowed mtus. > > > > Perhaps you want to take a look how to achieve that? ;) > > > Why not, consistency is good ... Thanks, 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
On Sun, Oct 6, 2013 at 4:13 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Sun, Oct 06, 2013 at 03:42:15PM +0100, Oussama Ghorbel wrote: >> The initialization in ip6gre_tnl_link_config is done as the following: >> static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu) >> { >> ... >> int addend = sizeof(struct ipv6hdr) + 4; >> ... >> /* Precalculate GRE options length */ >> if (t->parms.o_flags&(GRE_CSUM|GRE_KEY|GRE_SEQ)) { >> if (t->parms.o_flags&GRE_CSUM) >> addend += 4; >> if (t->parms.o_flags&GRE_KEY) >> addend += 4; >> if (t->parms.o_flags&GRE_SEQ) >> addend += 4; >> } >> ... >> dev->hard_header_len = rt->dst.dev->hard_header_len + addend; >> ... >> t->hlen = addend; >> .. >> } >> >> Unless they are other reasons, the hard_header_len is taken into >> account the GRE_KEY, GRE_SEQ .. > > But only if a new route is found. The hard_header_len reinitialization is > guarded by a (rt == NULL). We may have not found one on boot up. > In that case the function will make a return and hlen will be zero. Subtracting hlen in ip6gre_tunnel_change_mtu has no effect ... >> > To make this correct we would have to refactor the usage of the variables a >> > bit as is done in ipv4/ip_tunnel.c. The safest thing would be to leave this >> > check as-is currently although we exclude some allowed mtus. >> > >> > Perhaps you want to take a look how to achieve that? ;) >> > >> Why not, consistency is good ... > > Thanks, > > Hannes > Thanks, Oussama -- 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 Sun, Oct 06, 2013 at 04:36:56PM +0100, Oussama Ghorbel wrote: > On Sun, Oct 6, 2013 at 4:13 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > On Sun, Oct 06, 2013 at 03:42:15PM +0100, Oussama Ghorbel wrote: > >> The initialization in ip6gre_tnl_link_config is done as the following: > >> static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu) > >> { > >> ... > >> int addend = sizeof(struct ipv6hdr) + 4; > >> ... > >> /* Precalculate GRE options length */ > >> if (t->parms.o_flags&(GRE_CSUM|GRE_KEY|GRE_SEQ)) { > >> if (t->parms.o_flags&GRE_CSUM) > >> addend += 4; > >> if (t->parms.o_flags&GRE_KEY) > >> addend += 4; > >> if (t->parms.o_flags&GRE_SEQ) > >> addend += 4; > >> } > >> ... > >> dev->hard_header_len = rt->dst.dev->hard_header_len + addend; > >> ... > >> t->hlen = addend; > >> .. > >> } > >> > >> Unless they are other reasons, the hard_header_len is taken into > >> account the GRE_KEY, GRE_SEQ .. > > > > But only if a new route is found. The hard_header_len reinitialization is > > guarded by a (rt == NULL). We may have not found one on boot up. > > > In that case the function will make a return and hlen will be zero. > Subtracting hlen in ip6gre_tunnel_change_mtu has no effect ... Oh yes, somehow I missed that. We depend on t->hlen when pushing the gre header onto the skb. t->hlen == 0 should never be the case because we assume t->hlen > sizeof(struct ipv6_hdr). -- 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
Yes, to summarize, the idea of this patch was to fix the incoherence in the condition of ip6gre_tunnel_change_mtu function if (new_mtu < 68 || new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) From the ip6gre_tnl_link_config function we can see that: The variable addend is equal the ipv6 header + gre header (including the gre options) On the other hand hard_header_len equal to the header of the lower layer + addend. So the quantity - (dev->hard_header_len + tunnel->hlen) equals - (eth header + ipv6 header + gre header + ipv6 header + gre header) which by no means this would represent anything! (I've just taken ipv6 over ethernet as example) As we have seen there is another approach to fix this issue is to re-factor the hlen to hold only the length of gre as it's done for ipv4 gre, however the solution provided in the patch seems to be regression risk-less. Although the value hold by hlen is not coherent with the variable name nor with ipv4, I think there is an advantage of the current approach of ipv6 hlen over ipv4 hlen, because we save the calculation of ipv6 header each time. Ex: In ipv4 gre and in the function ipgre_header: iph = (struct iphdr *)skb_push(skb, t->hlen + sizeof(*iph)); In ipv6 and in the function ip6gre_header ipv6h = (struct ipv6hdr *)skb_push(skb, t->hlen); Thanks, Oussama On Sun, Oct 6, 2013 at 5:19 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Sun, Oct 06, 2013 at 04:36:56PM +0100, Oussama Ghorbel wrote: >> On Sun, Oct 6, 2013 at 4:13 PM, Hannes Frederic Sowa >> <hannes@stressinduktion.org> wrote: >> > On Sun, Oct 06, 2013 at 03:42:15PM +0100, Oussama Ghorbel wrote: >> >> The initialization in ip6gre_tnl_link_config is done as the following: >> >> static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu) >> >> { >> >> ... >> >> int addend = sizeof(struct ipv6hdr) + 4; >> >> ... >> >> /* Precalculate GRE options length */ >> >> if (t->parms.o_flags&(GRE_CSUM|GRE_KEY|GRE_SEQ)) { >> >> if (t->parms.o_flags&GRE_CSUM) >> >> addend += 4; >> >> if (t->parms.o_flags&GRE_KEY) >> >> addend += 4; >> >> if (t->parms.o_flags&GRE_SEQ) >> >> addend += 4; >> >> } >> >> ... >> >> dev->hard_header_len = rt->dst.dev->hard_header_len + addend; >> >> ... >> >> t->hlen = addend; >> >> .. >> >> } >> >> >> >> Unless they are other reasons, the hard_header_len is taken into >> >> account the GRE_KEY, GRE_SEQ .. >> > >> > But only if a new route is found. The hard_header_len reinitialization is >> > guarded by a (rt == NULL). We may have not found one on boot up. >> > >> In that case the function will make a return and hlen will be zero. >> Subtracting hlen in ip6gre_tunnel_change_mtu has no effect ... > > Oh yes, somehow I missed that. > > We depend on t->hlen when pushing the gre header onto the skb. t->hlen == 0 > should never be the case because we assume t->hlen > sizeof(struct ipv6_hdr). > -- 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 Sun, Oct 06, 2013 at 08:18:15PM +0100, Oussama Ghorbel wrote: > Yes, to summarize, the idea of this patch was to fix the incoherence > in the condition of ip6gre_tunnel_change_mtu function > > if (new_mtu < 68 || > new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) > > From the ip6gre_tnl_link_config function we can see that: > The variable addend is equal the ipv6 header + gre header (including > the gre options) > On the other hand hard_header_len equal to the header of the lower > layer + addend. > So the quantity - (dev->hard_header_len + tunnel->hlen) equals - (eth > header + ipv6 header + gre header + ipv6 header + gre header) which by > no means this would represent anything! (I've just taken ipv6 over > ethernet as example) > > As we have seen there is another approach to fix this issue is to > re-factor the hlen to hold only the length of gre as it's done for > ipv4 gre, however the solution provided in the patch seems to be > regression risk-less. I agree, it actually does not worsen the situation: Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > Although the value hold by hlen is not coherent with the variable name > nor with ipv4, I think there is an advantage of the current approach > of ipv6 hlen over ipv4 hlen, because we save the calculation of ipv6 > header each time. Ex: > In ipv4 gre and in the function ipgre_header: > iph = (struct iphdr *)skb_push(skb, t->hlen + sizeof(*iph)); > In ipv6 and in the function ip6gre_header > ipv6h = (struct ipv6hdr *)skb_push(skb, t->hlen); I see your point. But we should take care that t->hlen is always initialized, regardless if we got a route and outgoing device or not. 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
From: Oussama Ghorbel <ou.ghorbel@gmail.com> Date: Fri, 4 Oct 2013 10:52:13 +0100 > Unlike ipv4, the struct member hlen holds the length of the GRE and ipv6 > headers. This length is also counted in dev->hard_header_len. > Perhaps, it's more clean to modify the hlen to count only the GRE header > without ipv6 header as the variable name suggest, but the simple way to fix > this without regression risk is simply modify the calculation of the limit > in ip6gre_tunnel_change_mtu function. > Verified in kernel version v3.11. > > Signed-off-by: Oussama Ghorbel <ou.ghorbel@gmail.com> Please resubmit this with a proper Subject line. It should be "[PATCH] " then a legitimate subsystem prefix. In this case "ipv6: " would be appropriate. And then the "ipv6" in your existing Subject text is redundant so can be removed. Thanks. -- 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
OK, I've resubmitted the patch with the proper Subject line. The new mail subject is: [PATCH] ipv6: Fix the upper MTU limit in GRE tunnel Thanks, Oussama On Mon, Oct 7, 2013 at 5:53 PM, David Miller <davem@davemloft.net> wrote: > From: Oussama Ghorbel <ou.ghorbel@gmail.com> > Date: Fri, 4 Oct 2013 10:52:13 +0100 > >> Unlike ipv4, the struct member hlen holds the length of the GRE and ipv6 >> headers. This length is also counted in dev->hard_header_len. >> Perhaps, it's more clean to modify the hlen to count only the GRE header >> without ipv6 header as the variable name suggest, but the simple way to fix >> this without regression risk is simply modify the calculation of the limit >> in ip6gre_tunnel_change_mtu function. >> Verified in kernel version v3.11. >> >> Signed-off-by: Oussama Ghorbel <ou.ghorbel@gmail.com> > > Please resubmit this with a proper Subject line. > > It should be "[PATCH] " then a legitimate subsystem prefix. > In this case "ipv6: " would be appropriate. And then > the "ipv6" in your existing Subject text is redundant so > can be removed. > > Thanks. -- 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
From: Oussama Ghorbel <ou.ghorbel@gmail.com> Date: Mon, 7 Oct 2013 18:34:53 +0100 > OK, I've resubmitted the patch with the proper Subject line. > The new mail subject is: [PATCH] ipv6: Fix the upper MTU limit in GRE tunnel You forgot to propagate the "Acked-by: " tag that was given by reviewers of your patch. -- 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
Sorry for that. I've added it and I have resubmitted the patch. Thanks On Mon, Oct 7, 2013 at 6:38 PM, David Miller <davem@davemloft.net> wrote: > From: Oussama Ghorbel <ou.ghorbel@gmail.com> > Date: Mon, 7 Oct 2013 18:34:53 +0100 > >> OK, I've resubmitted the patch with the proper Subject line. >> The new mail subject is: [PATCH] ipv6: Fix the upper MTU limit in GRE tunnel > > You forgot to propagate the "Acked-by: " tag that was given by > reviewers of your patch. -- 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 Mon, Oct 7, 2013 at 3:02 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Sun, Oct 06, 2013 at 08:18:15PM +0100, Oussama Ghorbel wrote: >> Yes, to summarize, the idea of this patch was to fix the incoherence >> in the condition of ip6gre_tunnel_change_mtu function >> >> if (new_mtu < 68 || >> new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) >> >> From the ip6gre_tnl_link_config function we can see that: >> The variable addend is equal the ipv6 header + gre header (including >> the gre options) >> On the other hand hard_header_len equal to the header of the lower >> layer + addend. >> So the quantity - (dev->hard_header_len + tunnel->hlen) equals - (eth >> header + ipv6 header + gre header + ipv6 header + gre header) which by >> no means this would represent anything! (I've just taken ipv6 over >> ethernet as example) >> >> As we have seen there is another approach to fix this issue is to >> re-factor the hlen to hold only the length of gre as it's done for >> ipv4 gre, however the solution provided in the patch seems to be >> regression risk-less. > > I agree, it actually does not worsen the situation: > > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > >> Although the value hold by hlen is not coherent with the variable name >> nor with ipv4, I think there is an advantage of the current approach >> of ipv6 hlen over ipv4 hlen, because we save the calculation of ipv6 >> header each time. Ex: >> In ipv4 gre and in the function ipgre_header: >> iph = (struct iphdr *)skb_push(skb, t->hlen + sizeof(*iph)); >> In ipv6 and in the function ip6gre_header >> ipv6h = (struct ipv6hdr *)skb_push(skb, t->hlen); > > I see your point. But we should take care that t->hlen is always initialized, > regardless if we got a route and outgoing device or not. > OK, I'll investigate on this and I'll open a dedicated thread mail/send patch... > Greetings, > > Hannes > Thanks, Oussama -- 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
From: Oussama Ghorbel <ou.ghorbel@gmail.com> Date: Mon, 7 Oct 2013 18:52:15 +0100 > Sorry for that. I've added it and I have resubmitted the patch. Thank you. -- 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/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 90747f1..41487ab 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -1175,9 +1175,8 @@ done: static int ip6gre_tunnel_change_mtu(struct net_device *dev, int new_mtu) { - struct ip6_tnl *tunnel = netdev_priv(dev); if (new_mtu < 68 || - new_mtu > 0xFFF8 - dev->hard_header_len - tunnel->hlen) + new_mtu > 0xFFF8 - dev->hard_header_len) return -EINVAL; dev->mtu = new_mtu; return 0;
Unlike ipv4, the struct member hlen holds the length of the GRE and ipv6 headers. This length is also counted in dev->hard_header_len. Perhaps, it's more clean to modify the hlen to count only the GRE header without ipv6 header as the variable name suggest, but the simple way to fix this without regression risk is simply modify the calculation of the limit in ip6gre_tunnel_change_mtu function. Verified in kernel version v3.11. Signed-off-by: Oussama Ghorbel <ou.ghorbel@gmail.com> --- net/ipv6/ip6_gre.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)