netdev - Re: [PATCH net 1/2] ionic: add LIF_READY state to close probe-open race

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] [day] [month] [year] [list]

Date:   Wed, 29 Apr 2020 14:43:03 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] ionic: add LIF_READY state to close probe-open
 race

On 4/29/20 12:38 PM, David Miller wrote:
> From: Shannon Nelson <snelson@...sando.io>
> Date: Wed, 29 Apr 2020 11:37:38 -0700
>
>> Add a bit of state to the lif to signify when the queues are
>> ready to be used.  This closes an ionic_probe()/ionic_open()
>> race condition where the driver has registered the netdev
>> and signaled Link Up while running under ionic_probe(),
>> which NetworkManager or other user processes can see and then
>> try to bring up the device before the initial pass through
>> ionic_link_status_check() has finished.  NetworkManager's
>> thread can get into __dev_open() and set __LINK_STATE_START
>> in dev->state before the ionic_probe() thread makes it to the
>> netif_running() check, which results in the ionic_probe() thread
>> trying to start the queues before the queues have completed
>> their initialization.
>>
>> Adding a LIF_QREADY flag allows us to prevent this condition by
>> signaling whether the Tx/Rx queues are initialized and ready.
>>
>> Fixes: c672412f6172 ("ionic: remove lifs on fw reset")
>> Signed-off-by: Shannon Nelson <snelson@...sando.io>
> I would rather you fix this by adjusting the visibility.
>
> You should only call register_netdevice() when all of the device
> setup and software state initialization is complete.
>
> This improper ordering is the true cause of this bug and adding
> this new boolean state is simply papering over the core issue.
>
> Thanks.

It seems my impatience to see the Link Up message is a problem.  It 
looka as if I push the link check out of the probe thread and let the 
watchdog discover it later, all is much better.  I'll followup with a v2.

sln