[PATCH net] ip6_tunnel: fix GRE6 segmentation

[PATCH net] ip6_tunnel: fix GRE6 segmentation

[Jakub Kicinski]

Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
moved assiging inner_ipproto down from ipxip6_tnl_xmit() to
its callee ip6_tnl_xmit(). The latter is also used by GRE.

Since commit 38720352412a ("gre: Use inner_proto to obtain inner
header protocol") GRE had been depending on skb->inner_protocol
during segmentation. It sets it in gre_build_header() and reads
it in gre_gso_segment(). Changes to ip6_tnl_xmit() overwrite
the protocol, resulting in GSO skbs getting dropped.

Note that inner_protocol is a union with inner_ipproto,
GRE uses the former while the change switched it to the latter
(always setting it to just IPPROTO_GRE).

Restore the original location of skb_set_inner_ipproto(),
it is unclear why it was moved in the first place.

Fixes: 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ipv6/ip6_tunnel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 288bafded998..28ca70af014a 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1239,8 +1239,6 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
 	if (max_headroom > dev->needed_headroom)
 		dev->needed_headroom = max_headroom;
 
-	skb_set_inner_ipproto(skb, proto);
-
 	err = ip6_tnl_encap(skb, t, &proto, fl6);
 	if (err)
 		return err;
@@ -1377,6 +1375,8 @@ ipxip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev,
 	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
 		return -1;
 
+	skb_set_inner_ipproto(skb, protocol);
+
 	err = ip6_tnl_xmit(skb, dev, dsfield, &fl6, encap_limit, &mtu,
 			   protocol);
 	if (err != 0) {
-- 
2.31.1

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[Vadim Fedorenko]

On 22.06.2021 02:52, Jakub Kicinski wrote:
> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> moved assiging inner_ipproto down from ipxip6_tnl_xmit() to
> its callee ip6_tnl_xmit(). The latter is also used by GRE.
> 
> Since commit 38720352412a ("gre: Use inner_proto to obtain inner
> header protocol") GRE had been depending on skb->inner_protocol
> during segmentation. It sets it in gre_build_header() and reads
> it in gre_gso_segment(). Changes to ip6_tnl_xmit() overwrite
> the protocol, resulting in GSO skbs getting dropped.
> 
> Note that inner_protocol is a union with inner_ipproto,
> GRE uses the former while the change switched it to the latter
> (always setting it to just IPPROTO_GRE).
> 
> Restore the original location of skb_set_inner_ipproto(),
> it is unclear why it was moved in the first place.
> 
> Fixes: 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
Tested-by: Vadim Fedorenko <vfedorenko@novek.ru>

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[David Ahern]

On 6/21/21 7:52 PM, Jakub Kicinski wrote:
> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> moved assiging inner_ipproto down from ipxip6_tnl_xmit() to
> its callee ip6_tnl_xmit(). The latter is also used by GRE.
> 
> Since commit 38720352412a ("gre: Use inner_proto to obtain inner
> header protocol") GRE had been depending on skb->inner_protocol
> during segmentation. It sets it in gre_build_header() and reads
> it in gre_gso_segment(). Changes to ip6_tnl_xmit() overwrite
> the protocol, resulting in GSO skbs getting dropped.
> 
> Note that inner_protocol is a union with inner_ipproto,
> GRE uses the former while the change switched it to the latter
> (always setting it to just IPPROTO_GRE).
> 
> Restore the original location of skb_set_inner_ipproto(),
> it is unclear why it was moved in the first place.
> 
> Fixes: 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/ipv6/ip6_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

would be good to capture the GRE use case that found the bug and the
MPLS version as test cases under tools/testing/selftests/net. Both
should be doable using namespaces.

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Mon, 21 Jun 2021 18:52:54 -0700 you wrote:
> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> moved assiging inner_ipproto down from ipxip6_tnl_xmit() to
> its callee ip6_tnl_xmit(). The latter is also used by GRE.
> 
> Since commit 38720352412a ("gre: Use inner_proto to obtain inner
> header protocol") GRE had been depending on skb->inner_protocol
> during segmentation. It sets it in gre_build_header() and reads
> it in gre_gso_segment(). Changes to ip6_tnl_xmit() overwrite
> the protocol, resulting in GSO skbs getting dropped.
> 
> [...]

Here is the summary with links:
  - [net] ip6_tunnel: fix GRE6 segmentation
    https://git.kernel.org/netdev/net/c/a6e3f2985a80

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[Jakub Kicinski]

On Mon, 21 Jun 2021 22:28:05 -0600 David Ahern wrote:
> On 6/21/21 7:52 PM, Jakub Kicinski wrote:
> > Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> > moved assiging inner_ipproto down from ipxip6_tnl_xmit() to
> > its callee ip6_tnl_xmit(). The latter is also used by GRE.
> > 
> > Since commit 38720352412a ("gre: Use inner_proto to obtain inner
> > header protocol") GRE had been depending on skb->inner_protocol
> > during segmentation. It sets it in gre_build_header() and reads
> > it in gre_gso_segment(). Changes to ip6_tnl_xmit() overwrite
> > the protocol, resulting in GSO skbs getting dropped.
> > 
> > Note that inner_protocol is a union with inner_ipproto,
> > GRE uses the former while the change switched it to the latter
> > (always setting it to just IPPROTO_GRE).
> > 
> > Restore the original location of skb_set_inner_ipproto(),
> > it is unclear why it was moved in the first place.
> > 
> > Fixes: 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  net/ipv6/ip6_tunnel.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >   
> 
> would be good to capture the GRE use case that found the bug and the
> MPLS version as test cases under tools/testing/selftests/net. Both
> should be doable using namespaces.

I believe Vadim is working on MPLS side, how does this look for GRE?

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

# This test is for checking GRE GSO.

ret=0
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4

# all tests in this script. Can be overridden with -t option
TESTS="gre_gso"

VERBOSE=0
PAUSE_ON_FAIL=no
PAUSE=no
IP="ip -netns ns1"
NS_EXEC="ip netns exec ns1"
TMPFILE=`mktemp`
PID=

log_test()
{
	local rc=$1
	local expected=$2
	local msg="$3"

	if [ ${rc} -eq ${expected} ]; then
		printf "    TEST: %-60s  [ OK ]\n" "${msg}"
		nsuccess=$((nsuccess+1))
	else
		ret=1
		nfail=$((nfail+1))
		printf "    TEST: %-60s  [FAIL]\n" "${msg}"
		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
		echo
			echo "hit enter to continue, 'q' to quit"
			read a
			[ "$a" = "q" ] && exit 1
		fi
	fi

	if [ "${PAUSE}" = "yes" ]; then
		echo
		echo "hit enter to continue, 'q' to quit"
		read a
		[ "$a" = "q" ] && exit 1
	fi
}

setup()
{
	set -e
	ip netns add ns1
	ip netns set ns1 auto
	$IP link set dev lo up

	ip link add veth0 type veth peer name veth1
	ip link set veth0 up
	ip link set veth1 netns ns1
	$IP link set veth1 name veth0
	$IP link set veth0 up

	dd if=/dev/urandom of=$TMPFILE bs=1024 count=2048 &>/dev/null
	set +e
}

cleanup()
{
	rm -rf $TMPFILE
	[ -n "$PID" ] && kill $PID
	ip link del dev gre1 &> /dev/null
	ip link del dev veth0 &> /dev/null
	ip netns del ns1
}

get_linklocal()
{
	local dev=$1
	local ns=$2
	local addr

	[ -n "$ns" ] && ns="-netns $ns"

	addr=$(ip -6 -br $ns addr show dev ${dev} | \
	awk '{
		for (i = 3; i <= NF; ++i) {
			if ($i ~ /^fe80/)
				print $i
		}
	}'
	)
	addr=${addr/\/*}

	[ -z "$addr" ] && return 1

	echo $addr

	return 0
}

gre_create_tun()
{
	local a1=$1
	local a2=$2
	local mode

	[[ $a1 =~ ^[0-9.]*$ ]] && mode=gre || mode=ip6gre

	ip tunnel add gre1 mode $mode local $a1 remote $a2 dev veth0
	ip link set gre1 up
	$IP tunnel add gre1 mode $mode local $a2 remote $a1 dev veth0
	$IP link set gre1 up
}

gre_gst_test_checks()
{
	local name=$1
	local addr=$2

	$NS_EXEC nc -kl $port >/dev/null &
	PID=$!

	cat $TMPFILE | timeout 1 nc $addr $port
	log_test $? 0 "$name - copy file w/ TSO"

	ethtool -K veth0 tso off

	cat $TMPFILE | timeout 1 nc $addr $port
	log_test $? 0 "$name - copy file w/ GSO"

	ethtool -K veth0 tso on

	kill $PID
	PID=
}

gre6_gso_test()
{
	local port=7777

	setup

	a1=$(get_linklocal veth0)
	a2=$(get_linklocal veth0 ns1)

	gre_create_tun $a1 $a2

	ip  addr add 172.16.2.1/24 dev gre1
	$IP addr add 172.16.2.2/24 dev gre1

	ip  -6 addr add 2001:db8:1::1/64 dev gre1 nodad
	$IP -6 addr add 2001:db8:1::2/64 dev gre1 nodad

	sleep 2

	gre_gst_test_checks GREv6/v4 172.16.2.2
	gre_gst_test_checks GREv6/v6 2001:db8:1::2

	cleanup
}

gre_gso_test()
{
	gre6_gso_test
}

################################################################################
# usage

usage()
{
	cat <<EOF
usage: ${0##*/} OPTS

        -t <test>   Test(s) to run (default: all)
                    (options: $TESTS)
        -p          Pause on fail
        -P          Pause after each test before cleanup
        -v          verbose mode (show commands and output)
EOF
}

################################################################################
# main

while getopts :t:pPhv o
do
	case $o in
		t) TESTS=$OPTARG;;
		p) PAUSE_ON_FAIL=yes;;
		P) PAUSE=yes;;
		v) VERBOSE=$(($VERBOSE + 1));;
		h) usage; exit 0;;
		*) usage; exit 1;;
	esac
done

PEER_CMD="ip netns exec ${PEER_NS}"

# make sure we don't pause twice
[ "${PAUSE}" = "yes" ] && PAUSE_ON_FAIL=no

if [ "$(id -u)" -ne 0 ];then
	echo "SKIP: Need root privileges"
	exit $ksft_skip;
fi

if [ ! -x "$(command -v ip)" ]; then
	echo "SKIP: Could not run test without ip tool"
	exit $ksft_skip
fi

if [ ! -x "$(command -v nc)" ]; then
	echo "SKIP: Could not run test without nc tool"
	exit $ksft_skip
fi

# start clean
cleanup &> /dev/null

for t in $TESTS
do
	case $t in
	gre_gso)		gre_gso_test;;

	help) echo "Test names: $TESTS"; exit 0;;
	esac
done

if [ "$TESTS" != "none" ]; then
	printf "\nTests passed: %3d\n" ${nsuccess}
	printf "Tests failed: %3d\n"   ${nfail}
fi

exit $ret

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[David Ahern]

On 6/22/21 4:24 PM, Jakub Kicinski wrote:
>>>   
>>
>> would be good to capture the GRE use case that found the bug and the
>> MPLS version as test cases under tools/testing/selftests/net. Both
>> should be doable using namespaces.
> 
> I believe Vadim is working on MPLS side, how does this look for GRE?
> 

I like the template you followed. :-)

The test case looks good to me, thanks for doing it.

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[Jakub Kicinski]

On Tue, 22 Jun 2021 21:47:45 -0600 David Ahern wrote:
> On 6/22/21 4:24 PM, Jakub Kicinski wrote:
> >> would be good to capture the GRE use case that found the bug and the
> >> MPLS version as test cases under tools/testing/selftests/net. Both
> >> should be doable using namespaces.  
> > 
> > I believe Vadim is working on MPLS side, how does this look for GRE?
> 
> I like the template you followed. :-)

:)

> The test case looks good to me, thanks for doing it.

Noob question, why do we need that 2 sec wait with IPv6 sometimes?
I've seen it randomly in my local testing as well I wasn't sure if 
it's a bug or expected.

I make a v6 tunnel on top of a VLAN and for 2 secs after creation 
I get the wrong route in ip -6 r g.

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[David Ahern]

On 6/23/21 10:14 AM, Jakub Kicinski wrote:
> On Tue, 22 Jun 2021 21:47:45 -0600 David Ahern wrote:
>> On 6/22/21 4:24 PM, Jakub Kicinski wrote:
>>>> would be good to capture the GRE use case that found the bug and the
>>>> MPLS version as test cases under tools/testing/selftests/net. Both
>>>> should be doable using namespaces.  
>>>
>>> I believe Vadim is working on MPLS side, how does this look for GRE?
>>
>> I like the template you followed. :-)
> 
> :)
> 
>> The test case looks good to me, thanks for doing it.
> 
> Noob question, why do we need that 2 sec wait with IPv6 sometimes?
> I've seen it randomly in my local testing as well I wasn't sure if 
> it's a bug or expected.

It is to let IPv6 DAD to complete otherwise the address will not be
selected as a source address. That typically results in test failures.
There are sysctl settings that can prevent the race and hence the need
for the sleep.

> 
> I make a v6 tunnel on top of a VLAN and for 2 secs after creation 
> I get the wrong route in ip -6 r g.
> 

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[Guillaume Nault]

On Wed, Jun 23, 2021 at 12:28:05PM -0600, David Ahern wrote:
> On 6/23/21 10:14 AM, Jakub Kicinski wrote:
> > Noob question, why do we need that 2 sec wait with IPv6 sometimes?
> > I've seen it randomly in my local testing as well I wasn't sure if 
> > it's a bug or expected.
> 
> It is to let IPv6 DAD to complete otherwise the address will not be
> selected as a source address. That typically results in test failures.
> There are sysctl settings that can prevent the race and hence the need
> for the sleep.

But Jakub's script uses "nodad" in the "ip address add ..." commands.
Isn't that supposed to disable DAD entirely for the new address?
Why would it need an additional "sleep 2"?

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[David Ahern]

On 6/24/21 7:39 AM, Guillaume Nault wrote:
> On Wed, Jun 23, 2021 at 12:28:05PM -0600, David Ahern wrote:
>> On 6/23/21 10:14 AM, Jakub Kicinski wrote:
>>> Noob question, why do we need that 2 sec wait with IPv6 sometimes?
>>> I've seen it randomly in my local testing as well I wasn't sure if 
>>> it's a bug or expected.
>>
>> It is to let IPv6 DAD to complete otherwise the address will not be
>> selected as a source address. That typically results in test failures.
>> There are sysctl settings that can prevent the race and hence the need
>> for the sleep.
> 
> But Jakub's script uses "nodad" in the "ip address add ..." commands.
> Isn't that supposed to disable DAD entirely for the new address?
> Why would it need an additional "sleep 2"?
> 

it should yes. I think the selftests have acquired a blend of nodad,
sysctl and sleep. I'm sure it could be cleaned up and made consistent.

Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation

[Jakub Kicinski]

On Thu, 24 Jun 2021 08:36:12 -0600 David Ahern wrote:
> >> It is to let IPv6 DAD to complete otherwise the address will not be
> >> selected as a source address. That typically results in test failures.
> >> There are sysctl settings that can prevent the race and hence the need
> >> for the sleep.  
> > 
> > But Jakub's script uses "nodad" in the "ip address add ..." commands.
> > Isn't that supposed to disable DAD entirely for the new address?
> > Why would it need an additional "sleep 2"?
> >   
> 
> it should yes. I think the selftests have acquired a blend of nodad,
> sysctl and sleep. I'm sure it could be cleaned up and made consistent.

I was guessing that the DAD is happening on the link local address,
that's why, no? I'm using link local in the underlay.