[PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

The current code allows to inherit the TOS, TTL, DF from the payload
when skb->protocol is ETH_P_IP or ETH_P_IPV6.
However when the payload is VLAN encapsulated (e.g because the tunnel
is of type GRETAP), then this inheriting does not work, because the
visible skb->protocol is of type ETH_P_8021Q.

Add a check on ETH_P_8021Q and subsequently check the payload protocol.

Signed-off-by: Matthias May <matthias.may@westermo.com>
---
 net/ipv4/ip_tunnel.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 94017a8c3994..0dbd24861707 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -648,6 +648,12 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	u8 tos, ttl;
 	__be32 dst;
 	__be16 df;
+	__be16 *payload_protocol;
+
+	if (skb->protocol == htons(ETH_P_8021Q))
+		payload_protocol = (__be16 *)(skb->head + skb->network_header - 2);
+	else
+		payload_protocol = &skb->protocol;
 
 	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
 	connected = (tunnel->parms.iph.daddr != 0);
@@ -670,13 +676,12 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 			dst = tun_info->key.u.ipv4.dst;
 			md = true;
 			connected = true;
-		}
-		else if (skb->protocol == htons(ETH_P_IP)) {
+		} else if (*payload_protocol == htons(ETH_P_IP)) {
 			rt = skb_rtable(skb);
 			dst = rt_nexthop(rt, inner_iph->daddr);
 		}
 #if IS_ENABLED(CONFIG_IPV6)
-		else if (skb->protocol == htons(ETH_P_IPV6)) {
+		else if (*payload_protocol == htons(ETH_P_IPV6)) {
 			const struct in6_addr *addr6;
 			struct neighbour *neigh;
 			bool do_tx_error_icmp;
@@ -716,10 +721,10 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	tos = tnl_params->tos;
 	if (tos & 0x1) {
 		tos &= ~0x1;
-		if (skb->protocol == htons(ETH_P_IP)) {
+		if (*payload_protocol == htons(ETH_P_IP)) {
 			tos = inner_iph->tos;
 			connected = false;
-		} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		} else if (*payload_protocol == htons(ETH_P_IPV6)) {
 			tos = ipv6_get_dsfield((const struct ipv6hdr *)inner_iph);
 			connected = false;
 		}
@@ -765,7 +770,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	df = tnl_params->frag_off;
-	if (skb->protocol == htons(ETH_P_IP) && !tunnel->ignore_df)
+	if (*payload_protocol == htons(ETH_P_IP) && !tunnel->ignore_df)
 		df |= (inner_iph->frag_off & htons(IP_DF));
 
 	if (tnl_update_pmtu(dev, skb, rt, df, inner_iph, 0, 0, false)) {
@@ -786,10 +791,10 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	tos = ip_tunnel_ecn_encap(tos, inner_iph, skb);
 	ttl = tnl_params->ttl;
 	if (ttl == 0) {
-		if (skb->protocol == htons(ETH_P_IP))
+		if (*payload_protocol == htons(ETH_P_IP))
 			ttl = inner_iph->ttl;
 #if IS_ENABLED(CONFIG_IPV6)
-		else if (skb->protocol == htons(ETH_P_IPV6))
+		else if (*payload_protocol == htons(ETH_P_IPV6))
 			ttl = ((const struct ipv6hdr *)inner_iph)->hop_limit;
 #endif
 		else
-- 
2.35.1

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

Please make sure to CC folks pointed out by scripts/get_maintainer.pl

On Tue, 5 Jul 2022 16:54:42 +0200 Matthias May wrote:
> Subject: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

net-next may be more appropriate, since this never worked.
Unless it did, in which case we need a Fixes tag.

> The current code allows to inherit the TOS, TTL, DF from the payload
> when skb->protocol is ETH_P_IP or ETH_P_IPV6.
> However when the payload is VLAN encapsulated (e.g because the tunnel
> is of type GRETAP), then this inheriting does not work, because the
> visible skb->protocol is of type ETH_P_8021Q.
> 
> Add a check on ETH_P_8021Q and subsequently check the payload protocol.

Do we need to check for 8021AD as well?

> Signed-off-by: Matthias May <matthias.may@westermo.com>
> ---
>  net/ipv4/ip_tunnel.c | 21 +++++++++++++--------

Does ipv6 need the same treatment?

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1: Type: text/plain, Size: 1569 bytes --]


On 06/07/2022 03:25, Jakub Kicinski wrote:
> Please make sure to CC folks pointed out by scripts/get_maintainer.pl
> 
> On Tue, 5 Jul 2022 16:54:42 +0200 Matthias May wrote:
>> Subject: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames
> 
> net-next may be more appropriate, since this never worked.
> Unless it did, in which case we need a Fixes tag.
> 

I'm not aware that this ever worked. I tested back to 4.4.302.
Will send v2 to net-next.

>> The current code allows to inherit the TOS, TTL, DF from the payload
>> when skb->protocol is ETH_P_IP or ETH_P_IPV6.
>> However when the payload is VLAN encapsulated (e.g because the tunnel
>> is of type GRETAP), then this inheriting does not work, because the
>> visible skb->protocol is of type ETH_P_8021Q.
>>
>> Add a check on ETH_P_8021Q and subsequently check the payload protocol.
> 
> Do we need to check for 8021AD as well?
> 

Yeah that would make sense.
I can add the check for ETH_P_8021AD in v2.
Will have to find some hardware that is AD capable to test.

>> Signed-off-by: Matthias May <matthias.may@westermo.com>
>> ---
>>   net/ipv4/ip_tunnel.c | 21 +++++++++++++--------
> 
> Does ipv6 need the same treatment?

I don't think i changed anything regarding the behaviour for ipv6
by allowing to skip from the outer protocol to the payload protocol.
The previous code already
* got the TOS via ipv6_get_dsfield,
* the TTL was derived from the hop_limit,
* and DF does not exist for ipv6 so it doesn't check for ETH_P_IPV6.

BR
Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

On Wed, 6 Jul 2022 09:07:36 +0200 Matthias May wrote:
> >> The current code allows to inherit the TOS, TTL, DF from the payload
> >> when skb->protocol is ETH_P_IP or ETH_P_IPV6.
> >> However when the payload is VLAN encapsulated (e.g because the tunnel
> >> is of type GRETAP), then this inheriting does not work, because the
> >> visible skb->protocol is of type ETH_P_8021Q.
> >>
> >> Add a check on ETH_P_8021Q and subsequently check the payload protocol.  
> > 
> > Do we need to check for 8021AD as well?
> 
> Yeah that would make sense.
> I can add the check for ETH_P_8021AD in v2.
> Will have to find some hardware that is AD capable to test.

Why HW, you should be able to test with two Linux endpoints, no?

> >> Signed-off-by: Matthias May <matthias.may@westermo.com>
> >> ---
> >>   net/ipv4/ip_tunnel.c | 21 +++++++++++++--------  
> > 
> > Does ipv6 need the same treatment?  
> 
> I don't think i changed anything regarding the behaviour for ipv6
> by allowing to skip from the outer protocol to the payload protocol.

Sorry, to be clear what I meant - we try to enforce feature parity for
IPv6 these days in Linux. So I was asking if ipv6 needs changes to be
able to deal with VLANs. I think you got that but just in case.

> The previous code already
> * got the TOS via ipv6_get_dsfield,
> * the TTL was derived from the hop_limit,
> * and DF does not exist for ipv6 so it doesn't check for ETH_P_IPV6.

Purely by looking at the code I thought that VLAN-enabled GRETAP frames
would fall into ip6gre_xmit_other() which passes dsfield=0 into
__gre6_xmit(). key->tos only overrides the field for "external" tunnels, 
not normal tunnels with a dedicated netdev per tunnel.

A selftest to check both ipv4 and ipv6 would be the ultimate win there.

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1.1: Type: text/plain, Size: 2825 bytes --]

On 7/6/22 22:17, Jakub Kicinski wrote:
> On Wed, 6 Jul 2022 09:07:36 +0200 Matthias May wrote:
>>>> The current code allows to inherit the TOS, TTL, DF from the payload
>>>> when skb->protocol is ETH_P_IP or ETH_P_IPV6.
>>>> However when the payload is VLAN encapsulated (e.g because the tunnel
>>>> is of type GRETAP), then this inheriting does not work, because the
>>>> visible skb->protocol is of type ETH_P_8021Q.
>>>>
>>>> Add a check on ETH_P_8021Q and subsequently check the payload protocol.
>>>
>>> Do we need to check for 8021AD as well?
>>
>> Yeah that would make sense.
>> I can add the check for ETH_P_8021AD in v2.
>> Will have to find some hardware that is AD capable to test.
> 
> Why HW, you should be able to test with two Linux endpoints, no?
> 

Yes sure, that is how i did the initial version of the patch
(and yes, how i verified that AD works for v2).
My concern is, that if i use 2 devices that are patched identically,
that i may work with a bug present on both sides, but doesn't work
together with a 3rd party implementation.

>>>> Signed-off-by: Matthias May <matthias.may@westermo.com>
>>>> ---
>>>>    net/ipv4/ip_tunnel.c | 21 +++++++++++++--------
>>>
>>> Does ipv6 need the same treatment?
>>
>> I don't think i changed anything regarding the behaviour for ipv6
>> by allowing to skip from the outer protocol to the payload protocol.
> 
> Sorry, to be clear what I meant - we try to enforce feature parity for
> IPv6 these days in Linux. So I was asking if ipv6 needs changes to be
> able to deal with VLANs. I think you got that but just in case.
> 
>> The previous code already
>> * got the TOS via ipv6_get_dsfield,
>> * the TTL was derived from the hop_limit,
>> * and DF does not exist for ipv6 so it doesn't check for ETH_P_IPV6.
> 
> Purely by looking at the code I thought that VLAN-enabled GRETAP frames
> would fall into ip6gre_xmit_other() which passes dsfield=0 into
> __gre6_xmit(). key->tos only overrides the field for "external" tunnels,
> not normal tunnels with a dedicated netdev per tunnel.
> 

Ah you mean with IPv6 as transport for the tunnel.
I haven't really looked into that, since i have IPv6 disabled in my env.
I will try come up with a patch.

Since this does not touch the same code, should i send it as a separate
patch, or integrate it into this?

> A selftest to check both ipv4 and ipv6 would be the ultimate win there.

I'm not really familiar with selftests.
I've taken a look at some of the files in testing/selftests/net/ which
seems to be just a bunch of shell.
I'll see if i can write such a script that verifies this functionality.

Again as with the above about IPv6.
This seems to be kind of standalone. Should i integrate it into this
patch, or send as a separate?

BR
Matthias

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 683 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1.1: Type: text/plain, Size: 6263 bytes --]

On 7/6/22 22:17, Jakub Kicinski wrote:
> On Wed, 6 Jul 2022 09:07:36 +0200 Matthias May wrote:
>>>> The current code allows to inherit the TOS, TTL, DF from the payload
>>>> when skb->protocol is ETH_P_IP or ETH_P_IPV6.
>>>> However when the payload is VLAN encapsulated (e.g because the tunnel
>>>> is of type GRETAP), then this inheriting does not work, because the
>>>> visible skb->protocol is of type ETH_P_8021Q.
>>>>
>>>> Add a check on ETH_P_8021Q and subsequently check the payload protocol.
>>>
>>> Do we need to check for 8021AD as well?
>>
>> Yeah that would make sense.
>> I can add the check for ETH_P_8021AD in v2.
>> Will have to find some hardware that is AD capable to test.
> 
> Why HW, you should be able to test with two Linux endpoints, no?
> 
>>>> Signed-off-by: Matthias May <matthias.may@westermo.com>
>>>> ---
>>>>    net/ipv4/ip_tunnel.c | 21 +++++++++++++--------
>>>
>>> Does ipv6 need the same treatment?
>>
>> I don't think i changed anything regarding the behaviour for ipv6
>> by allowing to skip from the outer protocol to the payload protocol.
> 
> Sorry, to be clear what I meant - we try to enforce feature parity for
> IPv6 these days in Linux. So I was asking if ipv6 needs changes to be
> able to deal with VLANs. I think you got that but just in case.
> 
>> The previous code already
>> * got the TOS via ipv6_get_dsfield,
>> * the TTL was derived from the hop_limit,
>> * and DF does not exist for ipv6 so it doesn't check for ETH_P_IPV6.
> 
> Purely by looking at the code I thought that VLAN-enabled GRETAP frames
> would fall into ip6gre_xmit_other() which passes dsfield=0 into
> __gre6_xmit(). key->tos only overrides the field for "external" tunnels,
> not normal tunnels with a dedicated netdev per tunnel.
> 
> A selftest to check both ipv4 and ipv6 would be the ultimate win there.

I wrote the small test-script below.
Without looking at the code, to me it seems like setting the TOS when the outer
transport is IPv6 does not work.
This is on 5.19-rc5

When i create the tunnel with the script below:
outer=6
inner=4
and hardcode the TOS for the tunnel to 0xa0
ip link add name tep0 type ip6gretap local fdd1:ced0:5d88:3fce::1 remote fdd1:ced0:5d88:3fce::2 tos 0xa0
then i would expect the class of the resulting GRE frames to be 0xa0
however a ping through the tunnel shows me

$ tcpdump -i veth0 -n -v -e
15:24:01.395236 a2:f8:bb:1f:d3:b1 > 06:94:da:29:e0:0b, ethertype IPv6 (0x86dd), length 168: (flowlabel 0xb4c84, hlim 64, 
next-header unknown (60) payload length: 114) fdd1:ced0:5d88:3fce::1 > fdd1:ced0:5d88:3fce::2: DSTOPT (opt_type 0x04: 
len=1)(padn) GREv0, Flags [none], proto TEB (0x6558), length 106
	b2:d8:59:9d:ce:ca > b6:9e:a3:aa:f7:ae, ethertype 802.1Q (0x8100), length 102: vlan 99, p 0, ethertype IPv4 (0x0800), 
(tos 0x0, ttl 64, id 7056, offset 0, flags [DF], proto ICMP (1), length 84)
     198.19.0.1 > 198.19.0.2: ICMP echo request, id 59101, seq 5, length 64

whereas when i do a ping directly (not through the tunnel) with
ping fdd1:ced0:5d88:3fce::2 -Q 0xa0
then i see the class correctly

$ tcpdump -i veth0 -n -v -e
15:25:00.755188 a2:f8:bb:1f:d3:b1 > 06:94:da:29:e0:0b, ethertype IPv6 (0x86dd), length 118: (class 0xa0, flowlabel 
0x5f8c7, hlim 64, next-header ICMPv6 (58) payload length: 64) fdd1:ced0:5d88:3fce::1 > fdd1:ced0:5d88:3fce::2: [icmp6 
sum ok] ICMP6, echo request, id 46866, seq 2


Has setting the TOS for ip6gretap ever worked?
How should i go forward with this?
You state that your require feature parity between v4 and v6, but i am not sure i can provide that when
the underlying building blocks are missing.

BR
Matthias

---
#!/bin/sh

setup() {
	local outer inner test_namespace nsexec
	outer="$1"
	inner="$2"
	test_namespace="testing"
	nsexec="ip netns exec $test_namespace"

	# Create 'testing' netns, veth pair and connect main ns with testing ns
	ip netns add $test_namespace
	ip link add type veth
	ip link set veth1 netns testing
	ip link set veth0 up
	$nsexec ip link set veth1 up
	ip addr flush dev veth0
	$nsexec ip addr flush dev veth1

	# Create (ip6)gretap and assign outer IPv4/IPv6 addresses
	if [ "$outer" = "4" ]; then
		ip addr add 198.18.0.1/24 dev veth0
		$nsexec ip addr add 198.18.0.2/24 dev veth1
		ip link add name tep0 type gretap local 198.18.0.1 remote 198.18.0.2 tos inherit
		$nsexec ip link add name tep1 type gretap local 198.18.0.2 remote 198.18.0.1 tos inherit
	elif [ "$outer" = "6" ]; then
		ip addr add fdd1:ced0:5d88:3fce::1/64 dev veth0
		$nsexec ip addr add fdd1:ced0:5d88:3fce::2/64 dev veth1
		ip link add name tep0 type ip6gretap local fdd1:ced0:5d88:3fce::1 remote fdd1:ced0:5d88:3fce::2 tos inherit
		$nsexec ip link add name tep1 type ip6gretap local fdd1:ced0:5d88:3fce::2 remote fdd1:ced0:5d88:3fce::1 tos inherit
	else
		return -1
	fi

	# Bring (IP6)GRETAP link up and create VLAN on top
	ip link set tep0 up
	$nsexec ip link set tep1 up
	ip addr flush dev tep0
	$nsexec $nsexec ip addr flush dev tep1
	ip link add link tep0 name vlan99-0 type vlan id 99
	$nsexec ip link add link tep1 name vlan99-1 type vlan id 99
	ip link set vlan99-0 up
	$nsexec ip link set vlan99-1 up
	ip addr flush dev vlan99-0
	$nsexec ip addr flush dev vlan99-1

	# Assign inner IPv4/IPv6 addresses
	if [ "$inner" = "4" ]; then
		ip addr add 198.19.0.1/24 brd + dev vlan99-0
		$nsexec ip addr add 198.19.0.2/24 brd + dev vlan99-1
	elif [ "$inner" = "6" ]; then
		ip addr add fdd4:96cf:4eae:443b::1/64 dev vlan99-0
		$nsexec ip addr add fdd4:96cf:4eae:443b::2/64 dev vlan99-1
	else
		return -1
	fi
}

cleanup() {
	ip link del veth0
	ip netns del testing
	ip link del tep0
}
if [ "$1" != "start" ] && [ "$1" != "stop" ]; then
	echo "invalid first argument, valid is 'start' or 'stop'"
	exit 1
fi
if [ "$1" = "start" ] && [ "$2" != "4" ] && [ "$2" != "6" ]; then
	echo "invalid second argument (outer protocol), valid is '4' or '6'"
	exit 1
fi
if [ "$1" = "start" ] && [ "$3" != "4" ] && [ "$3" != "6" ]; then
	echo "invalid third argument (inner protocol), valid is '4' or '6'"
	exit 1
fi
if [ "$1" = "start" ]; then
	setup "$2" "$3"
elif [ "$1" = "stop" ]; then
	cleanup
fi


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 683 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

On Thu, 7 Jul 2022 13:57:11 +0200 Matthias May wrote:
> Again as with the above about IPv6.
> This seems to be kind of standalone. Should i integrate it into this
> patch, or send as a separate?

4 separate patches in one series would be best, I think.

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

On Thu, 7 Jul 2022 15:59:10 +0200 Matthias May wrote:
> Has setting the TOS for ip6gretap ever worked?

Not sure, looks like the code is there, but I haven't used it myself.
I presume "0xa0" is not trying to set the ECN bits? I never remember
which end has the ECN bits, those may well get nuked on the way.

> How should i go forward with this?

I think your example above shows that "tos 0xa0" does not work but the
conversation was about inheritance, does "tos inherit" not work either?

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1: Type: text/plain, Size: 1108 bytes --]


On 08/07/2022 02:01, Jakub Kicinski wrote:
> On Thu, 7 Jul 2022 15:59:10 +0200 Matthias May wrote:
>> Has setting the TOS for ip6gretap ever worked?
> 
> Not sure, looks like the code is there, but I haven't used it myself.
> I presume "0xa0" is not trying to set the ECN bits? I never remember
> which end has the ECN bits, those may well get nuked on the way.
> 
The ECN bits are the lowest 2 bits.
The DSCP value has to be a multiple of 4.

>> How should i go forward with this?
> 
> I think your example above shows that "tos 0xa0" does not work but the
> conversation was about inheritance, does "tos inherit" not work either?

Yes inherit does not work either. This is why i started setting it statically.
However I think I figured out what is going on.
Setting the TOS statically to 0xa0 does work... when the payload is IPv4 or IPv6,
which is also when inheriting works. For everything other type of payload, it is always 0x00.
This is different than with an IPv4 tunnel.
Should i consider this a bug that needs to be fixed, or is that the intended behaviour?

BR
Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

On Sat, 9 Jul 2022 22:09:12 +0200 Matthias May wrote:
> >> How should i go forward with this?  
> > 
> > I think your example above shows that "tos 0xa0" does not work but the
> > conversation was about inheritance, does "tos inherit" not work either?  
> 
> Yes inherit does not work either. This is why i started setting it statically.
> However I think I figured out what is going on.
> Setting the TOS statically to 0xa0 does work... when the payload is IPv4 or IPv6,
> which is also when inheriting works. For everything other type of payload, it is always 0x00.
> This is different than with an IPv4 tunnel.
> Should i consider this a bug that needs to be fixed, or is that the intended behaviour?

Yes, most likely a bug if you ask me :S

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1: Type: text/plain, Size: 1679 bytes --]

On 11/07/2022 20:29, Jakub Kicinski wrote:
> On Sat, 9 Jul 2022 22:09:12 +0200 Matthias May wrote:
>>>> How should i go forward with this?
>>>
>>> I think your example above shows that "tos 0xa0" does not work but the
>>> conversation was about inheritance, does "tos inherit" not work either?
>>
>> Yes inherit does not work either. This is why i started setting it statically.
>> However I think I figured out what is going on.
>> Setting the TOS statically to 0xa0 does work... when the payload is IPv4 or IPv6,
>> which is also when inheriting works. For everything other type of payload, it is always 0x00.
>> This is different than with an IPv4 tunnel.
>> Should i consider this a bug that needs to be fixed, or is that the intended behaviour?
> 
> Yes, most likely a bug if you ask me :S

Yeah i treated it as such, and sent a fix with the series.
I wasn't successful yet getting a selftest running.
I hope to get some feedback on the posted series, before i get the selftest running.
Especially this "bugfix", I'm not really sure if i did the right thing.
Essentially i just copied the prepare function for IPv6, stripped everything
out that i thought didn't fit and call it in the "other" path.
 From the manual tests i did, i couldn't find anything that i broke, but I'm not sure of that...

One thing that puzzles me a bit: Is there any reason why the IPv6 version of ip tunnels is so... distributed?
The IPv4 version does everything in a single function in ip_tunnels, while the IPv6 delegates some? of the parsing to
the respective tunnel types, but then does some of the parsing again in ip6_tunnel (e.g the ttl parsing).

BR
Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Nicolas Dichtel]

Le 12/07/2022 à 00:06, Matthias May a écrit :
[snip]
> One thing that puzzles me a bit: Is there any reason why the IPv6 version of ip
> tunnels is so... distributed?
Someone does the factorization for ipv4, but nobody for ipv6 ;-)

> The IPv4 version does everything in a single function in ip_tunnels, while the
> IPv6 delegates some? of the parsing to
> the respective tunnel types, but then does some of the parsing again in
> ip6_tunnel (e.g the ttl parsing).
Note that geneve and vxlan use ip_tunnel_get_dsfield() / ip_tunnel_get_ttl()
which also miss the vlan case.

Regards,
Nicolas

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1.1: Type: text/plain, Size: 869 bytes --]

On 7/12/22 09:17, Nicolas Dichtel wrote:
> Le 12/07/2022 à 00:06, Matthias May a écrit :
> [snip]
>> One thing that puzzles me a bit: Is there any reason why the IPv6 version of ip
>> tunnels is so... distributed?
> Someone does the factorization for ipv4, but nobody for ipv6 ;-)
> 
>> The IPv4 version does everything in a single function in ip_tunnels, while the
>> IPv6 delegates some? of the parsing to
>> the respective tunnel types, but then does some of the parsing again in
>> ip6_tunnel (e.g the ttl parsing).
> Note that geneve and vxlan use ip_tunnel_get_dsfield() / ip_tunnel_get_ttl()
> which also miss the vlan case.
> 
> Regards,
> Nicolas

Hi Nicolas

Yeah i feared as much.
My plan is to do the selftest for gretap, vxlan and geneve.
Are there any other tunnel types that can carry L2 that i don't know about?

BR
Matthias

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 683 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Nicolas Dichtel]

Le 12/07/2022 à 09:51, Matthias May a écrit :
> On 7/12/22 09:17, Nicolas Dichtel wrote:
>> Le 12/07/2022 à 00:06, Matthias May a écrit :
>> [snip]
>>> One thing that puzzles me a bit: Is there any reason why the IPv6 version of ip
>>> tunnels is so... distributed?
>> Someone does the factorization for ipv4, but nobody for ipv6 ;-)
>>
>>> The IPv4 version does everything in a single function in ip_tunnels, while the
>>> IPv6 delegates some? of the parsing to
>>> the respective tunnel types, but then does some of the parsing again in
>>> ip6_tunnel (e.g the ttl parsing).
>> Note that geneve and vxlan use ip_tunnel_get_dsfield() / ip_tunnel_get_ttl()
>> which also miss the vlan case.
>>
>> Regards,
>> Nicolas
> 
> Hi Nicolas
> 
> Yeah i feared as much.
> My plan is to do the selftest for gretap, vxlan and geneve.
> Are there any other tunnel types that can carry L2 that i don't know about?
I don't think of another kind of tunnel.


Thank you,
Nicolas

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1.1: Type: text/plain, Size: 13378 bytes --]

Hi

I finally got around to do the previously mentioned selftest for gretap, vxlan and geneve.
See the bash-script below.

Many of the vxlan/geneve tests are currently failing, with gretap working on net-next
because of the fixes i sent.
What is the policy on sending selftests that are failing?
Are fixes for the failures required in advance?

I'm not sure i can fix them.
Geneve seems to ignore the 3 upper bits of the DSCP completely.

My other concern is:
The whole test is... slow.
I tried to figure out what takes so long, and the culprit seem to be tcpdump.
It just takes ages to start capturing, more so when it is capturing IPv6.
Does anyone know of a better way to capture traffic and analyze it afterwards?
I used tcpdump because other tests seem to use it, and i guess this is a tool
that most everyone has installed (that works with networks).

BR
Matthias


--
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0

# Author: Matthias May <matthias.may@westermo.com>
#
# This script evaluates ip tunnels that are capable of carrying L2 traffic
# if they inherit or set the inheritable fields.
# Namely these tunnels are: 'gretap', 'vxlan' and 'geneve'.
# Checked inheritable fields are: TOS and TTL.
# The outer tunnel protocol of 'IPv4' or 'IPv6' is verified.
# As payload frames of type 'IPv4', 'IPv6' and 'other'(ARP) are verified.

if [ "$(id -u)" != "0" ]; then
	echo "Please run as root."
	exit 0
fi
if ! which tcpdump > /dev/null 2>&1; then
	echo "No tcpdump found. Required for this test."
	exit 0
fi

expected_tos="0x00"
expected_ttl="0"
failed=false

get_random_tos() {
	# Get a random hex tos value between 0x00 and 0xfc, a multiple of 4
	echo "0x$(tr -dc '0-9a-f' < /dev/urandom | head -c 1)\
$(tr -dc '048c' < /dev/urandom | head -c 1)"
}
get_random_ttl() {
	# Get a random dec value between 0 and 255
  	printf "%d" "0x$(tr -dc '0-9a-f' < /dev/urandom | head -c 2)"
}
get_field() {
	# Expects to get the 'head -n 1' of a captured frame by tcpdump.
	# Parses this first line and returns the specified field.
	local field="$1"
	local input="$2"
	local found=false
	input="$(echo "$input" | tr -d '(),')"
	for input_field in $input; do
		if $found; then
			echo "$input_field"
			return
		fi
		# The next field that we iterate over is the looked for value
		if [ "$input_field" = "$field" ]; then
			found=true
		fi
	done
	echo "0"
}
setup() {
	local type="$1"
	local outer="$2"
	local inner="$3"
	local tos_ttl="$4"
	local vlan="$5"
	local test_tos="0x00"
	local test_ttl="0"
	local ns="ip netns exec testing"

	# We don't want a test-tos of 0x00,
	# because this is the value that we get when no tos is set.
	expected_tos="$(get_random_tos)"
	while [ "$expected_tos" = "0x00" ]; do
		expected_tos="$(get_random_tos)"
	done
	if [ "$tos_ttl" = "random" ]; then
		test_tos="$expected_tos"
		tos="fixed $test_tos"
	elif [ "$tos_ttl" = "inherit" ]; then
		test_tos="$tos_ttl"
		tos="inherit $expected_tos"
	fi

	# We don't want a test-ttl of 64 or 0,
	# because 64 is when no ttl is set and 0 is not a valid ttl.
	expected_ttl="$(get_random_ttl)"
	while [ "$expected_ttl" = "64" ] || [ "$expected_ttl" = "0" ]; do
		expected_ttl="$(get_random_ttl)"
	done

	if [ "$tos_ttl" = "random" ]; then
		test_ttl="$expected_ttl"
		ttl="fixed $test_ttl"
	elif [ "$tos_ttl" = "inherit" ]; then
		test_ttl="$tos_ttl"
		ttl="inherit $expected_ttl"
	fi
	printf "│%7s │%6s │%6s │%13s │%13s │%6s │" \
	"$type" "$outer" "$inner" "$tos" "$ttl" "$vlan"

	# Create 'testing' netns, veth pair and connect main ns with testing ns
	ip netns add testing
	ip link add type veth
	ip link set veth1 netns testing
	ip link set veth0 up
	$ns ip link set veth1 up
	ip addr flush dev veth0
	$ns ip addr flush dev veth1

	local local_addr1=""
	local local_addr2=""
	if [ "$type" = "gretap" ] || [ "$type" = "vxlan" ]; then
		if [ "$outer" = "4" ]; then
			local_addr1="local 198.18.0.1"
			local_addr2="local 198.18.0.2"
		elif [ "$outer" = "6" ]; then
			local_addr1="local fdd1:ced0:5d88:3fce::1"
			local_addr2="local fdd1:ced0:5d88:3fce::2"
		fi
	fi
	local vxlan=""
	if [ "$type" = "vxlan" ]; then
		vxlan="vni 100 dstport 4789"
	fi
	local geneve=""
	if [ "$type" = "geneve" ]; then
		geneve="vni 100"
	fi
	# Create tunnel and assign outer IPv4/IPv6 addresses
	if [ "$outer" = "4" ]; then
		ip addr add 198.18.0.1/24 dev veth0
		$ns ip addr add 198.18.0.2/24 dev veth1
		ip link add name tep0 type $type $local_addr1 remote \
		198.18.0.2 tos $test_tos ttl $test_ttl $vxlan $geneve
		$ns ip link add name tep1 type $type $local_addr2 remote \
		198.18.0.1 tos $test_tos ttl $test_ttl $vxlan $geneve
	elif [ "$outer" = "6" ]; then
		if [ "$type" = "gretap" ]; then
			type="ip6gretap"
		fi
		ip addr add fdd1:ced0:5d88:3fce::1/64 dev veth0
		$ns ip addr add fdd1:ced0:5d88:3fce::2/64 dev veth1
  		ip link add name tep0 type $type $local_addr1 \
  		remote fdd1:ced0:5d88:3fce::2 tos $test_tos ttl $test_ttl \
  		$vxlan $geneve
  		$ns ip link add name tep1 type $type $local_addr2 \
  		remote fdd1:ced0:5d88:3fce::1 tos $test_tos ttl $test_ttl \
  		$vxlan $geneve
	fi

	# Bring L2-tunnel link up and create VLAN on top
	ip link set tep0 up
	$ns ip link set tep1 up
	ip addr flush dev tep0
	$ns ip addr flush dev tep1
	local parent
	if $vlan; then
		parent="vlan99-"
		ip link add link tep0 name ${parent}0 type vlan id 99
		$ns ip link add link tep1 name ${parent}1 type vlan id 99
		ip link set ${parent}0 up
		$ns ip link set ${parent}1 up
		ip addr flush dev ${parent}0
		$ns ip addr flush dev ${parent}1
	else
		parent="tep"
	fi

	# Assign inner IPv4/IPv6 addresses
	if [ "$inner" = "4" ] || [ "$inner" = "other" ]; then
		ip addr add 198.19.0.1/24 brd + dev ${parent}0
		$ns ip addr add 198.19.0.2/24 brd + dev ${parent}1
	elif [ "$inner" = "6" ]; then
		ip addr add fdd4:96cf:4eae:443b::1/64 dev ${parent}0
		$ns ip addr add fdd4:96cf:4eae:443b::2/64 dev ${parent}1
	fi
}

verify() {
	local outer="$1"
	local inner="$2"
	local tos_ttl="$3"
	local vlan="$4"

	local ping_pid out captured_tos captured_ttl result

	local ping_dst
	if [ "$inner" = "4" ]; then
		ping_dst="198.19.0.2"
	elif [ "$inner" = "6" ]; then
		ping_dst="fdd4:96cf:4eae:443b::2"
	elif [ "$inner" = "other" ]; then
		ping_dst="198.19.0.3" # Generates ARPs which are not IPv4/IPv6
	fi
	if [ "$tos_ttl" = "inherit" ]; then
		ping -i 0.1 $ping_dst -Q "$expected_tos" -t "$expected_ttl" \
		2>/dev/null 1>&2 & ping_pid="$!"
	else
		ping -i 0.1 $ping_dst 2>/dev/null 1>&2 & ping_pid="$!"
	fi
	local tunnel_type_offset tunnel_type_proto req_proto_offset req_offset
	if [ "$type" = "gretap" ]; then
		tunnel_type_proto="0x2f"
	elif [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
		tunnel_type_proto="0x11"
	fi
	if [ "$outer" = "4" ]; then
		tunnel_type_offset="9"
		if [ "$inner" = "4" ]; then
			req_proto_offset="47"
			req_offset="58"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 12))"
				req_offset="$((req_offset + 12))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip[$tunnel_type_offset] = $tunnel_type_proto and \
			ip[$req_proto_offset] = 0x01 and \
			ip[$req_offset] = 0x08 2>/dev/null | head -n 1)"
		elif [ "$inner" = "6" ]; then
			req_proto_offset="44"
			req_offset="78"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 12))"
				req_offset="$((req_offset + 12))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip[$tunnel_type_offset] = $tunnel_type_proto and \
			ip[$req_proto_offset] = 0x3a and \
			ip[$req_offset] = 0x80 2>/dev/null | head -n 1)"
		elif [ "$inner" = "other" ]; then
			req_proto_offset="36"
			req_offset="45"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 12))"
				req_offset="$((req_offset + 12))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			if [ "$tos_ttl" = "inherit" ]; then
				expected_tos="0x00"
				expected_ttl="64"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip[$tunnel_type_offset] = $tunnel_type_proto and \
			ip[$req_proto_offset] = 0x08 and \
			ip[$((req_proto_offset + 1))] = 0x06 and \
			ip[$req_offset] = 0x01 2>/dev/null | head -n 1)"
		fi
	elif [ "$outer" = "6" ]; then
		if [ "$type" = "gretap" ]; then
			tunnel_type_offset="40"
		elif [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
			tunnel_type_offset="6"
		fi
		if [ "$inner" = "4" ]; then
			local req_proto_offset="75"
			local req_offset="86"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip6[$tunnel_type_offset] = $tunnel_type_proto and \
			ip6[$req_proto_offset] = 0x01 and \
			ip6[$req_offset] = 0x08 2>/dev/null | head -n 1)"
		elif [ "$inner" = "6" ]; then
			local req_proto_offset="72"
			local req_offset="106"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip6[$tunnel_type_offset] = $tunnel_type_proto and \
			ip6[$req_proto_offset] = 0x3a and \
			ip6[$req_offset] = 0x80 2>/dev/null | head -n 1)"
		elif [ "$inner" = "other" ]; then
			local req_proto_offset="64"
			local req_offset="73"
			if [ "$type" = "vxlan" ] || [ "$type" = "geneve" ]; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			if $vlan; then
				req_proto_offset="$((req_proto_offset + 4))"
				req_offset="$((req_offset + 4))"
			fi
			if [ "$tos_ttl" = "inherit" ]; then
				expected_tos="0x00"
				expected_ttl="64"
			fi
			out="$(tcpdump -c 1 -v -i veth0 -n \
			ip6[$tunnel_type_offset] = $tunnel_type_proto and \
			ip6[$req_proto_offset] = 0x08 and \
			ip6[$((req_proto_offset + 1))] = 0x06 and \
			ip6[$req_offset] = 0x01 2>/dev/null | head -n 1)"
		fi
	fi
	kill -9 $ping_pid
	wait $ping_pid 2>/dev/null
	result="FAIL"
	if [ "$outer" = "4" ]; then
		captured_ttl="$(get_field "ttl" "$out")"
		captured_tos="$(printf "0x%02x" "$(get_field "tos" "$out")")"
		if [ "$captured_tos" = "$expected_tos" ] &&
		   [ "$captured_ttl" = "$expected_ttl" ]; then
			result="OK"
		fi
	elif [ "$outer" = "6" ]; then
		captured_ttl="$(get_field "hlim" "$out")"
		captured_tos="$(printf "0x%02x" "$(get_field "class" "$out")")"
		if [ "$captured_tos" = "$expected_tos" ] &&
		   [ "$captured_ttl" = "$expected_ttl" ]; then
			result="OK"
		fi
	fi

	printf "%7s │\n" "$result"
	if [ "$result" = "FAIL" ]; then
		failed=true
		if [ "$captured_tos" != "$expected_tos" ]; then
			printf "│%43s%27s │\n" \
			"Expected TOS value: $expected_tos" \
			"Captured TOS value: $captured_tos"
		fi
		if [ "$captured_ttl" != "$expected_ttl" ]; then
			printf "│%43s%27s │\n" \
			"Expected TTL value: $expected_ttl" \
			"Captured TTL value: $captured_ttl"
		fi
		printf "│%71s│\n" " "
	fi
}

cleanup() {
	ip link del veth0 2>/dev/null
	ip netns del testing 2>/dev/null
	ip link del tep0 2>/dev/null
}

printf "┌────────┬───────┬───────┬──────────────┬"
printf "──────────────┬───────┬────────┐\n"
for type in gretap vxlan geneve; do
  	for outer in 4 6; do
		printf "├────────┼───────┼───────┼──────────────┼"
		printf "──────────────┼───────┼────────┤\n"
		printf "│  Type  │ outer | inner │     tos      │"
		printf "      ttl     │  vlan │ result │\n"
		for inner in 4 6 other; do
			printf "├────────┼───────┼───────┼──────────────┼"
			printf "──────────────┼───────┼────────┤\n"
			for tos_ttl in inherit random; do
				for vlan in false true; do
					setup "$type" "$outer" "$inner" \
					"$tos_ttl" "$vlan"
					verify "$outer" "$inner" "$tos_ttl" \
					"$vlan"
					cleanup
				done
			done
		done
	done
done
printf "└────────┴───────┴───────┴──────────────┴"
printf "──────────────┴───────┴────────┘\n"

if $failed; then
	exit 1
fi

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 683 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Jakub Kicinski]

On Wed, 20 Jul 2022 17:24:17 +0200 Matthias May wrote:
> I finally got around to do the previously mentioned selftest for gretap, vxlan and geneve.
> See the bash-script below.
>
> Many of the vxlan/geneve tests are currently failing, with gretap working on net-next
> because of the fixes i sent.
> What is the policy on sending selftests that are failing?
> Are fixes for the failures required in advance?
> 
> I'm not sure i can fix them.
> Geneve seems to ignore the 3 upper bits of the DSCP completely.
> 
> My other concern is:
> The whole test is... slow.
> I tried to figure out what takes so long, and the culprit seem to be tcpdump.
> It just takes ages to start capturing, more so when it is capturing IPv6.
> Does anyone know of a better way to capture traffic and analyze it afterwards?
> I used tcpdump because other tests seem to use it, and i guess this is a tool
> that most everyone has installed (that works with networks).

Yeah, tcpdump is not great, there's a bunch of flags that make it a
little less bad (--immediate-mode?) 

Looking at the last test I wrote I ended up with:

tcpdump --immediate-mode -p -ni dummy0 -w $TMPF -c 4
sleep 0.05 # wait for tcpdump to start

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Nicolas Dichtel]

+ linux-kselftest@vger.kernel.org

Le 20/07/2022 à 17:24, Matthias May a écrit :
> Hi
> 
> I finally got around to do the previously mentioned selftest for gretap, vxlan
> and geneve.
> See the bash-script below.
> 
> Many of the vxlan/geneve tests are currently failing, with gretap working on
> net-next
> because of the fixes i sent.
> What is the policy on sending selftests that are failing?
> Are fixes for the failures required in advance?
I don't know, I've added linux-kselftest@vger.kernel.org to the thread.


Regards,
Nicolas

> 
> I'm not sure i can fix them.
> Geneve seems to ignore the 3 upper bits of the DSCP completely.

Re: [PATCH net] ip_tunnel: allow to inherit from VLAN encapsulated IP frames

[Matthias May]

[-- Attachment #1.1.1: Type: text/plain, Size: 1511 bytes --]

On 7/20/22 18:50, Jakub Kicinski wrote:
> On Wed, 20 Jul 2022 17:24:17 +0200 Matthias May wrote:
>> I finally got around to do the previously mentioned selftest for gretap, vxlan and geneve.
>> See the bash-script below.
>>
>> Many of the vxlan/geneve tests are currently failing, with gretap working on net-next
>> because of the fixes i sent.
>> What is the policy on sending selftests that are failing?
>> Are fixes for the failures required in advance?
>>
>> I'm not sure i can fix them.
>> Geneve seems to ignore the 3 upper bits of the DSCP completely.
>>
>> My other concern is:
>> The whole test is... slow.
>> I tried to figure out what takes so long, and the culprit seem to be tcpdump.
>> It just takes ages to start capturing, more so when it is capturing IPv6.
>> Does anyone know of a better way to capture traffic and analyze it afterwards?
>> I used tcpdump because other tests seem to use it, and i guess this is a tool
>> that most everyone has installed (that works with networks).
> 
> Yeah, tcpdump is not great, there's a bunch of flags that make it a
> little less bad (--immediate-mode?)
> 
> Looking at the last test I wrote I ended up with:
> 
> tcpdump --immediate-mode -p -ni dummy0 -w $TMPF -c 4
> sleep 0.05 # wait for tcpdump to start

Thank you for the hint with the immediate-mode, that already shaved over a minute off.

Before:
real	3m40.056s
user	0m4.103s
sys	0m2.434s

After:
real	2m34.438s
user	0m3.955s
sys	0m2.350s

BR
Matthias

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 683 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]