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 <[email protected]>
---
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;
--
1.7.9.5
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 <[email protected]>
> ---
> 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
On Sat, Oct 5, 2013 at 3:06 PM, Hannes Frederic Sowa
<[email protected]> 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 <[email protected]>
>> ---
>> 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
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
On Sun, Oct 6, 2013 at 4:13 PM, Hannes Frederic Sowa
<[email protected]> 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
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
> <[email protected]> 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).
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
<[email protected]> 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
>> <[email protected]> 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).
>
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 <[email protected]>
> 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
From: Oussama Ghorbel <[email protected]>
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 <[email protected]>
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.
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 <[email protected]> wrote:
> From: Oussama Ghorbel <[email protected]>
> 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 <[email protected]>
>
> 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.
From: Oussama Ghorbel <[email protected]>
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.
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 <[email protected]> wrote:
> From: Oussama Ghorbel <[email protected]>
> 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.
On Mon, Oct 7, 2013 at 3:02 AM, Hannes Frederic Sowa
<[email protected]> 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 <[email protected]>
>
>> 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
From: Oussama Ghorbel <[email protected]>
Date: Mon, 7 Oct 2013 18:52:15 +0100
> Sorry for that. I've added it and I have resubmitted the patch.
Thank you.