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  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

Powered by blists - more mailing lists