From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-22 01:53:01
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(-)
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>
---
From: David Ahern <hidden> Date: 2021-06-22 04:28:11
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.
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.
[...]
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-22 22:24:54
On Mon, 21 Jun 2021 22:28:05 -0600 David Ahern wrote:
On 6/21/21 7:52 PM, Jakub Kicinski wrote:
quoted
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
From: David Ahern <hidden> Date: 2021-06-23 03:47:51
On 6/22/21 4:24 PM, Jakub Kicinski wrote:
quoted
quoted
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-23 16:15:05
On Tue, 22 Jun 2021 21:47:45 -0600 David Ahern wrote:
On 6/22/21 4:24 PM, Jakub Kicinski wrote:
quoted
quoted
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.
From: David Ahern <hidden> Date: 2021-06-23 18:28:13
On 6/23/21 10:14 AM, Jakub Kicinski wrote:
On Tue, 22 Jun 2021 21:47:45 -0600 David Ahern wrote:
quoted
On 6/22/21 4:24 PM, Jakub Kicinski wrote:
quoted
quoted
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. :-)
:)
quoted
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.
On Wed, Jun 23, 2021 at 12:28:05PM -0600, David Ahern wrote:
On 6/23/21 10:14 AM, Jakub Kicinski wrote:
quoted
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"?
From: David Ahern <hidden> Date: 2021-06-24 14:36:17
On 6/24/21 7:39 AM, Guillaume Nault wrote:
On Wed, Jun 23, 2021 at 12:28:05PM -0600, David Ahern wrote:
quoted
On 6/23/21 10:14 AM, Jakub Kicinski wrote:
quoted
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-06-24 16:33:10
On Thu, 24 Jun 2021 08:36:12 -0600 David Ahern wrote:
quoted
quoted
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.