netdev - Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

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: <20180103164331.GA4926@splinter>
Date:   Wed, 3 Jan 2018 18:43:31 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     David Ahern <dsahern@...il.com>
Cc:     Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org,
        davem@...emloft.net, roopa@...ulusnetworks.com,
        nicolas.dichtel@...nd.com, mlxsw@...lanox.com
Subject: Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev
 up

On Wed, Jan 03, 2018 at 08:32:51AM -0700, David Ahern wrote:
> On 1/3/18 12:44 AM, Ido Schimmel wrote:
> >>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >>> index ed06b1190f05..b6405568ed7b 100644
> >>> --- a/net/ipv6/addrconf.c
> >>> +++ b/net/ipv6/addrconf.c
> >>> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> >>>  			if (run_pending)
> >>>  				addrconf_dad_run(idev);
> >>>  
> >>> +			/* Device has an address by now */
> >>> +			rt6_sync_up(dev, RTNH_F_DEAD);
> >>> +
> >>
> >> Seems like this should be in the NETDEV_UP section, say after
> >> addrconf_permanent_addr.
> > 
> > Unless the `keep_addr_on_down` sysctl is set, then at this stage the
> > netdev doesn't have an IP address and we shouldn't clear the dead flag
> > just yet.
> > 
> > This is consistent with IPv4 that clears the dead flag from nexthops in
> > a multipath route only if the nexthop device has an IP address. When the
> > last IPv4 address is removed from a netdev all the routes using it are
> > flushed and there's nothing to clear upon NETDEV_UP.
> 
> I have a bug about that IPv4 handling from the FRR team:
> 
> $ ip link add dummy1 type dummy
> $ ip li set dummy1 up
> $ ip route add 1.1.1.0/24 dev dummy1
> 
> $ ip addr add dev dummy1 2.2.2.1/24
> $ ip ro ls | grep dummy1
> 1.1.1.0/24 dev dummy1 scope link
> 2.2.2.0/24 dev dummy1 proto kernel scope link src 2.2.2.1
> 
> $ ip addr del dev dummy1 2.2.2.1/24
> $ ip ro ls | grep dummy1
> <no outpu>
> 
> The 1.1.1.0/24 route was removed as well the 2.2.2.0 connected route.

If you're going to skip the flushing in this case, at least mark the
nexthops as dead.

And this is my second reason to have rt6_sync_up() where I put it. I'm
preparing another set which sends FIB_EVENT_NH_ADD events from
rt6_sync_up() similar to what we've in fib_sync_up(). When mlxsw (others
in the future) processes the event it needs to add the nexthop back to
the forwarding plane. To do that, it needs to have a RIF for the
nexthop device. For the nexthop device to have a RIF, it needs at least
one IP address configured on the netdev.

Agree / disagree?