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

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

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.