netdev - Re: [PATCH net] atlantic: fix deadlock at aq_nic

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:   Sat, 15 Oct 2022 17:09:34 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Íñigo Huguet <ihuguet@...hat.com>
Cc:     irusskikh@...vell.com, dbogdanov@...vell.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, Li Liang <liali@...hat.com>
Subject: Re: [PATCH net] atlantic: fix deadlock at aq_nic_stop

> > Maybe the lock needs to be moved closer to what actually needs to be
> > protect? What is it protecting?
> 
> It's protecting the operations of aq_macsec_enable and aq_macsec_work.
> The locking was closer to them, but the idea of this patch is to move
> the locking to an earlier moment so, in the case we need to abort, do
> it before changing anything.

aq_check_txsa_expiration() seems to be one of the issues? At least,
the lock is taken before and released afterwards. So what in
aq_check_txsa_expiration() requires the lock?

I don't like the use of rtnl_trylock(). It suggests the basic design is
wrong, or overly complex, and so probably not working correctly.

https://blog.ffwll.ch/2022/07/locking-engineering.html

Please try to identify what is being protected. If it is driver
internal state, could it be replaced with a driver mutex, rather than
RTNL? Or is it network stack as a whole state, which really does
require RTNL? If so, how do other drivers deal with this problem? Is
it specific to MACSEC? Does MACSEC have a design problem?

   Andrew