When a nexthop is added, without a gw address, the default scope was set to 'host'. Thus, when a source address is selected, 127.0.0.1 may be chosen but rejected when the route is used.
When using a route without a nexthop id, the scope can be configured in the route, thus the problem doesn't exist.
To explain more deeply: when a user creates a nexthop, it cannot specify the scope. To create it, the function nh_create_ipv4() calls fib_check_nh() with scope set to 0. fib_check_nh() calls fib_check_nh_nongw() wich was setting scope to 'host'. Then, nh_create_ipv4() calls fib_info_update_nhc_saddr() with scope set to 'host'. The src addr is chosen before the route is inserted.
When a 'standard' route (ie without a reference to a nexthop) is added, fib_create_info() calls fib_info_update_nhc_saddr() with the scope set by the user. iproute2 set the scope to 'link' by default.
Here is a way to reproduce the problem: ip netns add foo ip -n foo link set lo up ip netns add bar ip -n bar link set lo up sleep 1
ip -n foo link add name eth0 type dummy ip -n foo link set eth0 up ip -n foo address add 192.168.0.1/24 dev eth0
ip -n foo link add name veth0 type veth peer name veth1 netns bar ip -n foo link set veth0 up ip -n bar link set veth1 up
ip -n bar address add 192.168.1.1/32 dev veth1 ip -n bar route add default dev veth1
ip -n foo nexthop add id 1 dev veth0 ip -n foo route add 192.168.1.1 nhid 1
Try to get/use the route:
$ ip -n foo route get 192.168.1.1 RTNETLINK answers: Invalid argument $ ip netns exec foo ping -c1 192.168.1.1 ping: connect: Invalid argument
Try without nexthop group (iproute2 sets scope to 'link' by dflt): ip -n foo route del 192.168.1.1 ip -n foo route add 192.168.1.1 dev veth0
Try to get/use the route:
$ ip -n foo route get 192.168.1.1 192.168.1.1 dev veth0 src 192.168.0.1 uid 0 cache $ ip netns exec foo ping -c1 192.168.1.1 PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data. 64 bytes from 192.168.1.1: icmp_seq=1 ttl=64 time=0.039 ms
--- 192.168.1.1 ping statistics --- 1 packets transmitted, 1 received, 0% packet loss, time 0ms rtt min/avg/max/mdev = 0.039/0.039/0.039/0.000 ms
CC: stable@vger.kernel.org Fixes: 597cfe4fc339 ("nexthop: Add support for IPv4 nexthops") Reported-by: Edwin Brossette edwin.brossette@6wind.com Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com ---
v1 -> v2: - remove useless arp off / fixed mac settings in the description
net/ipv4/fib_semantics.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index a57ba23571c9..20177ecf5bdd 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -1230,7 +1230,7 @@ static int fib_check_nh_nongw(struct net *net, struct fib_nh *nh,
nh->fib_nh_dev = in_dev->dev; dev_hold_track(nh->fib_nh_dev, &nh->fib_nh_dev_tracker, GFP_ATOMIC); - nh->fib_nh_scope = RT_SCOPE_HOST; + nh->fib_nh_scope = RT_SCOPE_LINK; if (!netif_carrier_ok(nh->fib_nh_dev)) nh->fib_nh_flags |= RTNH_F_LINKDOWN; err = 0;
This test implement the scenario described in the previous patch.
Signed-off-by: Nicolas Dichtel nicolas.dichtel@6wind.com ---
v1 -> v2: - add linux-kselftest@vger.kernel.org - clean trailing whitespaces - add a 'trap' on exit - remove useless sleep - remove useless arp off / fixed mac settings
tools/testing/selftests/net/Makefile | 2 +- .../selftests/net/fib_nexthop_nongw.sh | 119 ++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/net/fib_nexthop_nongw.sh
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index ddad703ace34..db05b3764b77 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -11,7 +11,7 @@ TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh TEST_PROGS += test_vxlan_fdb_changelink.sh so_txtime.sh ipv6_flowlabel.sh TEST_PROGS += tcp_fastopen_backup_key.sh fcnal-test.sh l2tp.sh traceroute.sh -TEST_PROGS += fin_ack_lat.sh fib_nexthop_multiprefix.sh fib_nexthops.sh +TEST_PROGS += fin_ack_lat.sh fib_nexthop_multiprefix.sh fib_nexthops.sh fib_nexthop_nongw.sh TEST_PROGS += altnames.sh icmp.sh icmp_redirect.sh ip6_gre_headroom.sh TEST_PROGS += route_localnet.sh TEST_PROGS += reuseaddr_ports_exhausted.sh diff --git a/tools/testing/selftests/net/fib_nexthop_nongw.sh b/tools/testing/selftests/net/fib_nexthop_nongw.sh new file mode 100755 index 000000000000..b7b928b38ce4 --- /dev/null +++ b/tools/testing/selftests/net/fib_nexthop_nongw.sh @@ -0,0 +1,119 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# ns: h1 | ns: h2 +# 192.168.0.1/24 | +# eth0 | +# | 192.168.1.1/32 +# veth0 <---|---> veth1 +# Validate source address selection for route without gateway + +PAUSE_ON_FAIL=no +VERBOSE=0 +ret=0 + +################################################################################ +# helpers + +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 + + [ "$VERBOSE" = "1" ] && echo +} + +run_cmd() +{ + local cmd="$*" + local out + local rc + + if [ "$VERBOSE" = "1" ]; then + echo "COMMAND: $cmd" + fi + + out=$(eval $cmd 2>&1) + rc=$? + if [ "$VERBOSE" = "1" -a -n "$out" ]; then + echo "$out" + fi + + [ "$VERBOSE" = "1" ] && echo + + return $rc +} + +################################################################################ +# config +setup() +{ + ip netns add h1 + ip -n h1 link set lo up + ip netns add h2 + ip -n h2 link set lo up + + # Add a fake eth0 to support an ip address + ip -n h1 link add name eth0 type dummy + ip -n h1 link set eth0 up + ip -n h1 address add 192.168.0.1/24 dev eth0 + + # Configure veths (same @mac, arp off) + ip -n h1 link add name veth0 type veth peer name veth1 netns h2 + ip -n h1 link set veth0 up + + ip -n h2 link set veth1 up + + # Configure @IP in the peer netns + ip -n h2 address add 192.168.1.1/32 dev veth1 + ip -n h2 route add default dev veth1 + + # Add a nexthop without @gw and use it in a route + ip -n h1 nexthop add id 1 dev veth0 + ip -n h1 route add 192.168.1.1 nhid 1 +} + +cleanup() +{ + ip netns del h1 2>/dev/null + ip netns del h2 2>/dev/null +} + +trap cleanup EXIT + +################################################################################ +# main + +while getopts :pv o +do + case $o in + p) PAUSE_ON_FAIL=yes;; + v) VERBOSE=1;; + esac +done + +cleanup +setup + +run_cmd ip -netns h1 route get 192.168.1.1 +log_test $? 0 "nexthop: get route with nexthop without gw" +run_cmd ip netns exec h1 ping -c1 192.168.1.1 +log_test $? 0 "nexthop: ping through nexthop without gw" + +exit $ret
On Tue, Jul 12, 2022 at 11:55:45AM +0200, Nicolas Dichtel wrote:
This test implement the scenario described in the previous patch.
"previous patch" does not work well when things are committed to the kernel tree. Please be descriptive.
thanks,
greg k-h
On Tue, 12 Jul 2022 12:14:17 +0200 Greg KH wrote:
On Tue, Jul 12, 2022 at 11:55:45AM +0200, Nicolas Dichtel wrote:
This test implement the scenario described in the previous patch.
"previous patch" does not work well when things are committed to the kernel tree. Please be descriptive.
And please don't resend your patches in reply to the previous version. Add a lore link to the previous version in the commit message if you want. In-reply-to breaks the review ordering for us :/
Le 13/07/2022 à 02:25, Jakub Kicinski a écrit :
On Tue, 12 Jul 2022 12:14:17 +0200 Greg KH wrote:
On Tue, Jul 12, 2022 at 11:55:45AM +0200, Nicolas Dichtel wrote:
This test implement the scenario described in the previous patch.
"previous patch" does not work well when things are committed to the kernel tree. Please be descriptive.
And please don't resend your patches in reply to the previous version. Add a lore link to the previous version in the commit message if you want. In-reply-to breaks the review ordering for us :/
Oh ok, I didn't know that.
On Wed, 13 Jul 2022 09:36:37 +0200 Nicolas Dichtel wrote:
And please don't resend your patches in reply to the previous version. Add a lore link to the previous version in the commit message if you want. In-reply-to breaks the review ordering for us :/
Oh ok, I didn't know that.
Yeah, I haven't documented it because it's a bit of an oddity and frankly a shortcoming of the tooling on my side. But IDK how to "detach" the threads in a way that'd allow me to keep a queue sorted by posting data :(
Le 12/07/2022 à 12:14, Greg KH a écrit :
On Tue, Jul 12, 2022 at 11:55:45AM +0200, Nicolas Dichtel wrote:
This test implement the scenario described in the previous patch.
"previous patch" does not work well when things are committed to the kernel tree. Please be descriptive.
Ok, no problem. Note that patches order of a series is preserved, in network tree at least. And because I don't have a sha1 right now, it seemed to me the best way to uniquely identify a commit ;-)
Regards, Nicolas
linux-kselftest-mirror@lists.linaro.org
-
Greg KH -
Jakub Kicinski -
Nicolas Dichtel