[PATCH net 1/2] ip: fix dflt addr selection for connected nexthop

[PATCH net 1/2] ip: fix dflt addr selection for connected nexthop

[Nicolas Dichtel]

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 address 00:09:c0:26:05:82
ip -n foo link set veth0 arp off
ip -n foo link set veth0 up
ip -n bar link set veth1 address 00:09:c0:26:05:82
ip -n bar link set veth1 arp off
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>
---
 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;
-- 
2.33.0

[PATCH net 2/2] selftests/net: test nexthop without gw

[Nicolas Dichtel]

This test implement the scenario described in the previous patch.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 tools/testing/selftests/net/Makefile          |   2 +-
 .../selftests/net/fib_nexthop_nongw.sh        | 125 ++++++++++++++++++
 2 files changed, 126 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..6e82562eaf4a
--- /dev/null
+++ b/tools/testing/selftests/net/fib_nexthop_nongw.sh
@@ -0,0 +1,125 @@
+#!/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
+	sleep 1
+
+	# 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 address 00:09:c0:26:05:82
+	ip -n h1 link set veth0 arp off
+	ip -n h1 link set veth0 up
+
+	ip -n h2 link set veth1 address 00:09:c0:26:05:82
+	ip -n h2 link set veth1 arp off
+	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
+}
+
+################################################################################
+# main
+
+while getopts :pv o
+do
+	case $o in
+		p) PAUSE_ON_FAIL=yes;;
+		v) VERBOSE=1;;
+	esac
+done
+
+cleanup
+setup
+sleep 2
+
+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"
+
+cleanup
+
+exit $ret
-- 
2.33.0

Re: [PATCH net 2/2] selftests/net: test nexthop without gw

[Paolo Abeni]

On Wed, 2022-07-06 at 18:05 +0200, Nicolas Dichtel wrote:
> This test implement the scenario described in the previous patch.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  tools/testing/selftests/net/Makefile          |   2 +-
>  .../selftests/net/fib_nexthop_nongw.sh        | 125 ++++++++++++++++++
>  2 files changed, 126 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..6e82562eaf4a
> --- /dev/null
> +++ b/tools/testing/selftests/net/fib_nexthop_nongw.sh
> @@ -0,0 +1,125 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# ns: h1               | ns: h2           

Trailing whitespace above

> +#   192.168.0.1/24     |
> +#            eth0      |
> +#                      |       192.168.1.1/32
> +#            veth0 <---|---> veth1        

same here.

> +# 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
> +	sleep 1

Why is this needed here? same question for the 'sleep 2' after the
setup.

> +
> +	# 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 address 00:09:c0:26:05:82
> +	ip -n h1 link set veth0 arp off

As in the example in the previous commit, I suggest to drop the
apparently not relevant  'arp off'/ static macs 

> +	ip -n h1 link set veth0 up
> +
> +	ip -n h2 link set veth1 address 00:09:c0:26:05:82
> +	ip -n h2 link set veth1 arp off
> +	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
> +}

This become more roboust if you add a

trap cleanup EXIT

additionally, with the above you could remove the explicit cleanups
below

> +
> +################################################################################
> +# main
> +
> +while getopts :pv o
> +do
> +	case $o in
> +		p) PAUSE_ON_FAIL=yes;;
> +		v) VERBOSE=1;;
> +	esac
> +done
> +
> +cleanup
> +setup
> +sleep 2
> +
> +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"
> +
> +cleanup
> +
> +exit $ret

Re: [PATCH net 2/2] selftests/net: test nexthop without gw

[Nicolas Dichtel]

Le 12/07/2022 à 10:19, Paolo Abeni a écrit :
[snip]
>> +################################################################################
>> +# config
>> +setup()
>> +{
>> +	ip netns add h1
>> +	ip -n h1 link set lo up
>> +	ip netns add h2
>> +	ip -n h2 link set lo up
>> +	sleep 1
> 
> Why is this needed here? same question for the 'sleep 2' after the
> setup.

The 'sleep 2' after the setup was 'copy & paste'. I will remove it.

The bug was initially spotted in 'init_net' and when I first tried to reproduce
it with netns, I didn't succeed without the 'sleep 1'. I didn't analyzed more
deeply. In fact, when I replay the test now, it fails as expected.
Let's remove it also.

[PATCH net v2 1/2] ip: fix dflt addr selection for connected nexthop

[Nicolas Dichtel]

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;
-- 
2.33.0

[PATCH net v2 2/2] selftests/net: test nexthop without gw

[Nicolas Dichtel]

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
-- 
2.33.0

Re: [PATCH net v2 2/2] selftests/net: test nexthop without gw

[Greg KH]

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

Re: [PATCH net v2 2/2] selftests/net: test nexthop without gw

[Jakub Kicinski]

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 :/

Re: [PATCH net v2 2/2] selftests/net: test nexthop without gw

[Nicolas Dichtel]

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.

Re: [PATCH net v2 2/2] selftests/net: test nexthop without gw

[Jakub Kicinski]

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 :(

Re: [PATCH net v2 2/2] selftests/net: test nexthop without gw

[Nicolas Dichtel]

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

Re: [PATCH net 1/2] ip: fix dflt addr selection for connected nexthop

[Paolo Abeni]

On Wed, 2022-07-06 at 18:05 +0200, Nicolas Dichtel wrote:
> 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 address 00:09:c0:26:05:82
> ip -n foo link set veth0 arp off

It looks like the 'arp off'/fixed mac address is not relevant for the
test case, could you please drop it, so that the example and the self-
test are more clean?

> ip -n foo link set veth0 up
> ip -n bar link set veth1 address 00:09:c0:26:05:82
> ip -n bar link set veth1 arp off
> 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")

Why that commit? It looks like fib_check_nh() used SCOPE_HOST for nongw
next hop since well before ?!?

Otherwise LGTM.

Paolo

Re: [PATCH net 1/2] ip: fix dflt addr selection for connected nexthop

[Nicolas Dichtel]

Le 12/07/2022 à 10:11, Paolo Abeni a écrit :
[snip]
>>
>> ip -n foo link add name veth0 type veth peer name veth1 netns bar
>> ip -n foo link set veth0 address 00:09:c0:26:05:82
>> ip -n foo link set veth0 arp off
> 
> It looks like the 'arp off'/fixed mac address is not relevant for the
> test case, could you please drop it, so that the example and the self-
> test are more clean?Good point, I will remove these lines.

[snip]
>> Fixes: 597cfe4fc339 ("nexthop: Add support for IPv4 nexthops")
> 
> Why that commit? It looks like fib_check_nh() used SCOPE_HOST for nongw
> next hop since well before ?!?
Yes, but with "standard" route, ie when the nexthop objects are not used, the
scope used, at the end, is the one specified in the netlink message.
With nexthop object, the user cannot specify the scope and we end up in this
problem.
Thus, the problem exists only with nexthop object only.

I tried to explain this difference in the commit log, but maybe it's not clear
enough.

> 
> Otherwise LGTM.


Thank you,
Nicolas