IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[Konstantin Shemyak]

The kernel has taken the decision to always enforce DF bit on IPv4 
tunnels, which have fixed (not inherited) TTL (e.g. 
net/ipv4/ipip.c:ipip_tunnel_ioctl()). Commment by Alexey Kuznetsov in 
the head of ip_gre.c explains that the reason is attempting to avoid 
network loops.

But the commit c54419321455631 removed this enforcing from GRE tunnels,
not changing this behavior for IP-IP (net/ipv4/ipip.c) and SIT 
(net/ipv6/sit.c).

It can be discussed whether such enforcing of DF bit is exactly the
desired behavior, but shouldn't it at least be identical across IPv4 
tunnels?

Konstantin Shemyak

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[Hannes Frederic Sowa]

Hello,

On Thu, Nov 26, 2015, at 19:28, Konstantin Shemyak wrote:
> The kernel has taken the decision to always enforce DF bit on IPv4 
> tunnels, which have fixed (not inherited) TTL (e.g. 
> net/ipv4/ipip.c:ipip_tunnel_ioctl()). Commment by Alexey Kuznetsov in 
> the head of ip_gre.c explains that the reason is attempting to avoid 
> network loops.
> 
> But the commit c54419321455631 removed this enforcing from GRE tunnels,
> not changing this behavior for IP-IP (net/ipv4/ipip.c) and SIT 
> (net/ipv6/sit.c).
> 
> It can be discussed whether such enforcing of DF bit is exactly the
> desired behavior, but shouldn't it at least be identical across IPv4 
> tunnels?

Very simple, I would like to see DF bit being enforced in case we have a
static TTL. Parvin, any reasons you removed this code? It is currently
the only way to make sure the network does not kill itself in an endless
loop on configuration mistakes.

Thanks for noticing,
Hannes

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[Konstantin Shemyak]

On 01.12.2015 12:15, Hannes Frederic Sowa wrote:
> Hello,
>
> On Thu, Nov 26, 2015, at 19:28, Konstantin Shemyak wrote:
>> The kernel has taken the decision to always enforce DF bit on IPv4
>> tunnels, which have fixed (not inherited) TTL (e.g.
>> net/ipv4/ipip.c:ipip_tunnel_ioctl()). Commment by Alexey Kuznetsov in
>> the head of ip_gre.c explains that the reason is attempting to avoid
>> network loops.
>>
>> But the commit c54419321455631 removed this enforcing from GRE tunnels,
>> not changing this behavior for IP-IP (net/ipv4/ipip.c) and SIT
>> (net/ipv6/sit.c).
>>
>> It can be discussed whether such enforcing of DF bit is exactly the
>> desired behavior, but shouldn't it at least be identical across IPv4
>> tunnels?
>
> Very simple, I would like to see DF bit being enforced in case we have a
> static TTL. Parvin, any reasons you removed this code? It is currently
> the only way to make sure the network does not kill itself in an endless
> loop on configuration mistakes.

This solution does not completely eliminate all network loops, as there 
may be other routers in the loop which do not set the DF bit. But it is 
true that this behavior is "we did all we could".
My point was not to question its feasibility, but to make it similar 
across GRE, IP-IP and SIT tunnels.

Konstantin.

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[Hannes Frederic Sowa]

On Tue, Dec 1, 2015, at 14:20, Konstantin Shemyak wrote:
> On 01.12.2015 12:15, Hannes Frederic Sowa wrote:
> > Hello,
> >
> > On Thu, Nov 26, 2015, at 19:28, Konstantin Shemyak wrote:
> >> The kernel has taken the decision to always enforce DF bit on IPv4
> >> tunnels, which have fixed (not inherited) TTL (e.g.
> >> net/ipv4/ipip.c:ipip_tunnel_ioctl()). Commment by Alexey Kuznetsov in
> >> the head of ip_gre.c explains that the reason is attempting to avoid
> >> network loops.
> >>
> >> But the commit c54419321455631 removed this enforcing from GRE tunnels,
> >> not changing this behavior for IP-IP (net/ipv4/ipip.c) and SIT
> >> (net/ipv6/sit.c).
> >>
> >> It can be discussed whether such enforcing of DF bit is exactly the
> >> desired behavior, but shouldn't it at least be identical across IPv4
> >> tunnels?
> >
> > Very simple, I would like to see DF bit being enforced in case we have a
> > static TTL. Parvin, any reasons you removed this code? It is currently
> > the only way to make sure the network does not kill itself in an endless
> > loop on configuration mistakes.
> 
> This solution does not completely eliminate all network loops, as there 
> may be other routers in the loop which do not set the DF bit. But it is 
> true that this behavior is "we did all we could".

That is what I meant. We cannot care about other routers, at least Linux
shouldn't be pinpointed down to be the problem. :)

> My point was not to question its feasibility, but to make it similar 
> across GRE, IP-IP and SIT tunnels.

I would send a patch to add it again if Parvin didn't have good reasons
to remove it.

Thanks,
Hannes

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Tue, 01 Dec 2015 14:30:55 +0100

> On Tue, Dec 1, 2015, at 14:20, Konstantin Shemyak wrote:
>> My point was not to question its feasibility, but to make it similar 
>> across GRE, IP-IP and SIT tunnels.
> 
> I would send a patch to add it again if Parvin didn't have good reasons
> to remove it.

The tunnel code consolidation created a lot of regressions and subtle
unintended changes in behavior between the different tunnel types.

This DF bit issue is just yet another example of that.

I'm really disappointed at how many bugs and problems were introduced
by those changes.

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[Konstantin Shemyak]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On 01.12.2015 19:08, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Tue, 01 Dec 2015 14:30:55 +0100
>
>> On Tue, Dec 1, 2015, at 14:20, Konstantin Shemyak wrote:
>>> My point was not to question its feasibility, but to make it similar
>>> across GRE, IP-IP and SIT tunnels.
>>
>> I would send a patch to add it again if Parvin didn't have good reasons
>> to remove it.
>
> The tunnel code consolidation created a lot of regressions and subtle
> unintended changes in behavior between the different tunnel types.
>
> This DF bit issue is just yet another example of that.

The patch is rather trivial; attached.

Konstantin.

[-- Attachment #2: 0001-Set-DF-bit-to-GRE-tunnels-with-fixed-TTL-similarly-t.patch --]
[-- Type: text/x-patch, Size: 951 bytes --]

>From b8a901a0d970907a2dc162744536b1ef07e5d45a Mon Sep 17 00:00:00 2001
From: Konstantin Shemyak <konstantin@shemyak.com>
Date: Sat, 5 Dec 2015 17:15:55 +0200
Subject: [PATCH] Set DF bit to GRE tunnels with fixed TTL, similarly to IP-IP
 and SIT

The DF bit on IPv4 tunnels with fixed TTL is enforced in order to
prevent networking loops. This was the original behavior of GRE
tunnels, lost in the refactoring.
---
 net/ipv4/ip_gre.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6145214..485bf27 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -679,6 +679,10 @@ static int ipgre_tunnel_ioctl(struct net_device *dev,
 		    ((p.i_flags|p.o_flags)&(GRE_VERSION|GRE_ROUTING)))
 			return -EINVAL;
 	}
+
+        if (p.iph.ttl)
+                p.iph.frag_off |= htons(IP_DF);
+
 	p.i_flags = gre_flags_to_tnl_flags(p.i_flags);
 	p.o_flags = gre_flags_to_tnl_flags(p.o_flags);
 
-- 
1.9.1

Re: IPv4 tunnels: why IP-IP and SIT enforce DF bit, but GRE does not?

[David Miller]

From: Konstantin Shemyak <konstantin@shemyak.com>
Date: Sat, 5 Dec 2015 17:24:39 +0200

> On 01.12.2015 19:08, David Miller wrote:
>> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Date: Tue, 01 Dec 2015 14:30:55 +0100
>>
>>> On Tue, Dec 1, 2015, at 14:20, Konstantin Shemyak wrote:
>>>> My point was not to question its feasibility, but to make it similar
>>>> across GRE, IP-IP and SIT tunnels.
>>>
>>> I would send a patch to add it again if Parvin didn't have good
>>> reasons
>>> to remove it.
>>
>> The tunnel code consolidation created a lot of regressions and subtle
>> unintended changes in behavior between the different tunnel types.
>>
>> This DF bit issue is just yet another example of that.
> 
> The patch is rather trivial; attached.

No signoff, so I can't apply this.

Also patches should be submitted as fresh mailing list postings,
with the patch inline rather than as an attachment.

Thanks.