netdev - Re: [PATCH net v2] ipv6: Handle race in addrconf_dad

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]

Date:   Wed, 2 Oct 2019 17:37:52 -0600
From:   David Ahern <dsahern@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, davem@...emloft.net,
        jakub.kicinski@...ronome.com
Cc:     netdev@...r.kernel.org, rajendra.dendukuri@...adcom.com
Subject: Re: [PATCH net v2] ipv6: Handle race in addrconf_dad_work

On 10/2/19 5:11 PM, Eric Dumazet wrote:
> 
> 
> On 10/2/19 3:36 PM, Eric Dumazet wrote:
>>
>>
>> On 10/2/19 3:33 PM, David Ahern wrote:
> 
>>>
>>> I flipped to IF_READY based on addrconf_ifdown and idev checks seeming
>>> more appropriate.
>>>
>>
> 
> Note that IF_READY is set in ipv6_add_dev() if all these conditions are true :
> 
> if (netif_running(dev) && addrconf_link_ready(dev))
>      ndev->if_flags |= IF_READY;
> 
> So maybe in my setup IF_READY is set later from a notifier event ?
> 

ipv6_add_dev is called to initialize the ipv6 struct for the device. In
most cases it is invoked when a device is registered (NETDEV_REGISTER in
addrconf_notify).

IF_READY was added by commit 3c21edbd1137 explicitly to delay DAD if the
underlying link is not up (ie., carrier up not just admin up). Given
that it seems appropriate to key off that flag in addrconf_dad_work.

IF_READY is only set or reset under rtnl. It is set on NETDEV_UP (admin
up) or NETDEV_CHANGE (e.g., carrier up) and reset for NETDEV_DOWN. The
latter case is what the patch was trying to detect and bail on.

Maybe dad was getting kicked off too early before (IF_READY not set) and
now it stalls because of the early IF_READY check and nothing restarts
it ...