mbox series

[RFC,net,0/2] nexthop: Do not flush blackhole nexthops when loopback goes down

Message ID 20210228142613.1642938-1-idosch@idosch.org
Headers show
Series nexthop: Do not flush blackhole nexthops when loopback goes down | expand

Message

Ido Schimmel Feb. 28, 2021, 2:26 p.m. UTC
From: Ido Schimmel <idosch@nvidia.com>

Patch #1 prevents blackhole nexthops from being flushed when the
loopback device goes down given that as far as user space is concerned,
these nexthops do not have a nexthop device.

Patch #2 adds a test case.

This is a user visible change, so sending as RFC.

Personally, I think it is worth making the change. The flow is quite
obscure and therefore unlikely to result in any regressions, especially
when the nexthop API is quite new compared to the legacy API. In
addition, the current behavior is very puzzling to those not familiar
with the inner workings of the nexthop code.

Regardless, there are no regressions in fib_nexthops.sh with this
change:

 # ./fib_nexthops.sh
 ...
 Tests passed: 165
 Tests failed:   0

Ido Schimmel (2):
  nexthop: Do not flush blackhole nexthops when loopback goes down
  selftests: fib_nexthops: Test blackhole nexthops when loopback goes
    down

 net/ipv4/nexthop.c                          | 10 +++++++---
 tools/testing/selftests/net/fib_nexthops.sh |  8 ++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

David Ahern Feb. 28, 2021, 11:41 p.m. UTC | #1
On 2/28/21 7:26 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Test that blackhole nexthops are not flushed when the loopback device
> goes down.
> 


> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  tools/testing/selftests/net/fib_nexthops.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index 4c7d33618437..d98fb85e201c 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -1524,6 +1524,14 @@ basic()
>  	run_cmd "$IP nexthop replace id 2 blackhole dev veth1"
>  	log_test $? 2 "Blackhole nexthop with other attributes"
>  
> +	# blackhole nexthop should not be affected by the state of the loopback
> +	# device
> +	run_cmd "$IP link set dev lo down"
> +	check_nexthop "id 2" "id 2 blackhole"
> +	log_test $? 0 "Blackhole nexthop with loopback device down"
> +
> +	run_cmd "$IP link set dev lo up"
> +
>  	#
>  	# groups
>  	#
> 

Thanks for adding a test.

Reviewed-by: David Ahern <dsahern@gmail.com>