netdev - Re: [PATCHv3 net] ipv6: do not match device when remove source route

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZLzhMDIayD2z4szG@shredder>
Date: Sun, 23 Jul 2023 11:13:36 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Thomas Haller <thaller@...hat.com>
Subject: Re: [PATCHv3 net] ipv6: do not match device when remove source route

On Fri, Jul 21, 2023 at 04:59:21PM +0800, Hangbin Liu wrote:
> Hi Ido,
> 
> On Thu, Jul 20, 2023 at 05:49:47PM +0300, Ido Schimmel wrote:
> > Actually, there is another problem here. IPv4 checks that the address is
> > indeed gone (it can be assigned to more than one interface):
> > 
> > + ip link add name dummy1 up type dummy
> > + ip link add name dummy2 up type dummy
> > + ip link add name dummy3 up type dummy
> > + ip address add 192.0.2.1/24 dev dummy1
> > + ip address add 192.0.2.1/24 dev dummy2
> > + ip route add 198.51.100.0/24 dev dummy3 src 192.0.2.1
> > + ip address del 192.0.2.1/24 dev dummy2
> > + ip -4 r s
> > 192.0.2.0/24 dev dummy1 proto kernel scope link src 192.0.2.1 
> > 198.51.100.0/24 dev dummy3 scope link src 192.0.2.1 
> > 
> > But it doesn't happen for IPv6:
> > 
> > + ip link add name dummy1 up type dummy
> > + ip link add name dummy2 up type dummy
> > + ip link add name dummy3 up type dummy
> > + ip address add 2001:db8:1::1/64 dev dummy1
> > + ip address add 2001:db8:1::1/64 dev dummy2
> > + ip route add 2001:db8:2::/64 dev dummy3 src 2001:db8:1::1
> > + ip address del 2001:db8:1::1/64 dev dummy2
> > + ip -6 r s
> > 2001:db8:1::/64 dev dummy1 proto kernel metric 256 pref medium
> > 2001:db8:2::/64 dev dummy3 metric 1024 pref medium
> > fe80::/64 dev dummy1 proto kernel metric 256 pref medium
> > fe80::/64 dev dummy2 proto kernel metric 256 pref medium
> > fe80::/64 dev dummy3 proto kernel metric 256 pref medium
> 
> Hmm, what kind of usage that need to add same address on different interfaces?

I don't know, but when I checked the code and tested it I noticed that
the kernel doesn't care on which interface the address is configured.
Therefore, in order for deletion to be consistent with addition and with
IPv4, the preferred source address shouldn't be removed from routes in
the VRF table as long as the address is configured on one of the
interfaces in the VRF.

> BTW, to fix it, how about check if the IPv6 addr still exist. e.g.
> 
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4590,10 +4590,11 @@ 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 &&
> +       if (rt != net->ipv6.fib6_null_entry &&
> +           rt->fib6_table->tb6_id == tb6_id &&
> +           !ipv6_chk_addr_and_flags(net, addr, NULL, true, 0, IFA_F_TENTATIVE) &&
>             ipv6_addr_equal(addr, &rt->fib6_prefsrc.addr)) {

ipv6_chk_addr_and_flags() is more expensive than ipv6_addr_equal() so
better to first check that route indeed uses the address as the
preferred source address and only then check if it exists.

Maybe you can even do it in rt6_remove_prefsrc(). That would be similar
to what IPv4 is doing.

>                 spin_lock_bh(&rt6_exception_lock);
>                 /* remove prefsrc entry */
> 
> Thanks
> Hangbin