* [PATCH net] ip6_tunnel: fix GRE6 segmentation
@ 2021-06-22 1:52 Jakub Kicinski
2021-06-22 1:58 ` Vadim Fedorenko
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-06-22 1:52 UTC (permalink / raw
To: davem; +Cc: netdev, yoshfuji, dsahern, vfedorenko, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-22 1:52 [PATCH net] ip6_tunnel: fix GRE6 segmentation Jakub Kicinski
@ 2021-06-22 1:58 ` Vadim Fedorenko
2021-06-22 4:28 ` David Ahern
2021-06-22 17:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 11+ messages in thread
From: Vadim Fedorenko @ 2021-06-22 1:58 UTC (permalink / raw
To: Jakub Kicinski, davem; +Cc: netdev, yoshfuji, dsahern
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-22 1:52 [PATCH net] ip6_tunnel: fix GRE6 segmentation Jakub Kicinski
2021-06-22 1:58 ` Vadim Fedorenko
@ 2021-06-22 4:28 ` David Ahern
2021-06-22 22:24 ` Jakub Kicinski
2021-06-22 17:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2021-06-22 4:28 UTC (permalink / raw
To: Jakub Kicinski, davem; +Cc: netdev, yoshfuji, dsahern, vfedorenko
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-22 1:52 [PATCH net] ip6_tunnel: fix GRE6 segmentation Jakub Kicinski
2021-06-22 1:58 ` Vadim Fedorenko
2021-06-22 4:28 ` David Ahern
@ 2021-06-22 17:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-06-22 17:40 UTC (permalink / raw
To: Jakub Kicinski; +Cc: davem, netdev, yoshfuji, dsahern, vfedorenko
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-22 4:28 ` David Ahern
@ 2021-06-22 22:24 ` Jakub Kicinski
2021-06-23 3:47 ` David Ahern
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-06-22 22:24 UTC (permalink / raw
To: David Ahern; +Cc: davem, netdev, yoshfuji, dsahern, vfedorenko
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-22 22:24 ` Jakub Kicinski
@ 2021-06-23 3:47 ` David Ahern
2021-06-23 16:14 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2021-06-23 3:47 UTC (permalink / raw
To: Jakub Kicinski; +Cc: davem, netdev, yoshfuji, dsahern, vfedorenko
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-23 3:47 ` David Ahern
@ 2021-06-23 16:14 ` Jakub Kicinski
2021-06-23 18:28 ` David Ahern
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-06-23 16:14 UTC (permalink / raw
To: David Ahern; +Cc: davem, netdev, yoshfuji, dsahern, vfedorenko
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-23 16:14 ` Jakub Kicinski
@ 2021-06-23 18:28 ` David Ahern
2021-06-24 13:39 ` Guillaume Nault
0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2021-06-23 18:28 UTC (permalink / raw
To: Jakub Kicinski; +Cc: davem, netdev, yoshfuji, dsahern, vfedorenko
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.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-23 18:28 ` David Ahern
@ 2021-06-24 13:39 ` Guillaume Nault
2021-06-24 14:36 ` David Ahern
0 siblings, 1 reply; 11+ messages in thread
From: Guillaume Nault @ 2021-06-24 13:39 UTC (permalink / raw
To: David Ahern; +Cc: Jakub Kicinski, davem, netdev, yoshfuji, dsahern, vfedorenko
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"?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-24 13:39 ` Guillaume Nault
@ 2021-06-24 14:36 ` David Ahern
2021-06-24 16:33 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2021-06-24 14:36 UTC (permalink / raw
To: Guillaume Nault
Cc: Jakub Kicinski, davem, netdev, yoshfuji, dsahern, vfedorenko
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] ip6_tunnel: fix GRE6 segmentation
2021-06-24 14:36 ` David Ahern
@ 2021-06-24 16:33 ` Jakub Kicinski
0 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-06-24 16:33 UTC (permalink / raw
To: David Ahern; +Cc: Guillaume Nault, davem, netdev, yoshfuji, dsahern, vfedorenko
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-24 16:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-22 1:52 [PATCH net] ip6_tunnel: fix GRE6 segmentation Jakub Kicinski
2021-06-22 1:58 ` Vadim Fedorenko
2021-06-22 4:28 ` David Ahern
2021-06-22 22:24 ` Jakub Kicinski
2021-06-23 3:47 ` David Ahern
2021-06-23 16:14 ` Jakub Kicinski
2021-06-23 18:28 ` David Ahern
2021-06-24 13:39 ` Guillaume Nault
2021-06-24 14:36 ` David Ahern
2021-06-24 16:33 ` Jakub Kicinski
2021-06-22 17:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.