Re: [PATCHv5 net-next] ipv6: do not match device when remove source route

[Ido Schimmel <idosch@idosch.org>]

On Fri, Aug 11, 2023 at 05:53:08PM +0800, Hangbin Liu wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 64e873f5895f..0f981cc5bed1 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4590,11 +4590,12 @@ static int fib6_remove_prefsrc(struct fib6_info *rt, void *arg)
>  	struct net_device *dev = ((struct arg_dev_net_ip *)arg)->dev;
>  	struct net *net = ((struct arg_dev_net_ip *)arg)->net;
>  	struct in6_addr *addr = ((struct arg_dev_net_ip *)arg)->addr;
> +	u32 tb6_id = l3mdev_fib_table(dev) ? : RT_TABLE_MAIN;
>  
> -	if (!rt->nh &&
> -	    ((void *)rt->fib6_nh->fib_nh_dev == dev || !dev) &&
> -	    rt != net->ipv6.fib6_null_entry &&
> -	    ipv6_addr_equal(addr, &rt->fib6_prefsrc.addr)) {
> +	if (rt != net->ipv6.fib6_null_entry &&
> +	    rt->fib6_table->tb6_id == tb6_id &&
> +	    ipv6_addr_equal(addr, &rt->fib6_prefsrc.addr) &&
> +	    !ipv6_chk_addr(net, addr, rt->fib6_nh->fib_nh_dev, 0)) {
>  		spin_lock_bh(&rt6_exception_lock);
>  		/* remove prefsrc entry */
>  		rt->fib6_prefsrc.plen = 0;

The table check is incorrect which is what I was trying to explain here
[1]. The route insertion code does not check that the preferred source
is accessible from the VRF where the route is installed, but instead
that it is accessible from the VRF of the first nexthop device. I'm not
going to debate whether it is correct or not. I'm going to say that the
logic should be consistent between the route insertion and deletion
paths. That is, if I'm only able to insert a route with a preferred
source address because some address exists, then when this address is
removed the preferred source address should be removed from the route.

Here is an example with your patch applied:

+ ip link add name dummy1 up type dummy
+ ip link add name vrf1 up type vrf table 1111
+ ip link set dev dummy1 master vrf1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
Error: Invalid source address.
+ ip address add 2001:db8:1::1/64 dev dummy1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium
+ ip address del 2001:db8:1::1/64 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium

Note how it is not possible to add the route to the main table because
the address does not exist, but then after the address is deleted the
route still exists with the preferred source address.

And this is the same example, but with the patch from [1]:

+ ip link add name dummy1 up type dummy
+ ip link add name vrf1 up type vrf table 1111
+ ip link set dev dummy1 master vrf1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
Error: Invalid source address.
+ ip address add 2001:db8:1::1/64 dev dummy1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium
+ ip address del 2001:db8:1::1/64 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 metric 1024 pref medium

[1] https://lore.kernel.org/netdev/ZNSol%2F7x5oI6amEB@shredder/