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]

Message-ID: <CACT4oudiN82eWw+cpB8K12TKGdNjyB74OcYEq9kn13GpT+=ndg@mail.gmail.com>
Date:   Tue, 18 Oct 2022 08:11:10 +0200
From:   Íñigo Huguet <ihuguet@...hat.com>
To:     Andrew Lunn <andrew@...n.ch>
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

On Tue, Oct 18, 2022 at 2:28 AM Andrew Lunn <andrew@...n.ch> wrote:
> Did you look at other drivers using MACSEC offload? Is this driver
> unique in having stuff run in a work queue which you need to cancel?
> In fact, it is not limited to MACSEC, it could be any work queue which
> holds RTNL and needs to be cancelled.

Yes, I did.

About other drivers using MACSEC offload (which are only 2): they
don't have work or anything similar related to macsec where they need
to take rtnl_lock or any other lock. But in this driver the need of a
lock seems justified, at least as far as I can understand.

About other drivers having works that need to take rtnl_lock: they
cancel it in PCI shutdown/remove functions, then call
unregister_netdev, acquiring rtnl_lock. I considered doing the same,
but I didn't for 2 reasons:
1. There is no need to have a periodic task running for a stopped NIC
2. The task uses some resources that are deinitialized at NIC stop and
try to communicate with NIC's firmware. If it's stopped in a way
different to PCI shutdown/remove (i.e. ip link set down) the task
would continue to be executed and try to use these deinitialized
resources and communicate with a stopped hw.

Of course that point 2 possibly can be fixed to avoid doing it if NIC
has been stopped, but it still remains point 1. I didn't research if
other drivers really need to have the task running periodically even
with the NIC stopped, but I certainly know that this one doesn't need
it, looking at what the task does.

I do appreciate feedback, suggestions and changes requests (actually I
happily accepted to send v2 according to them, right?). But I'd rather
if they contained more specific proposals and examples of what I can
do to improve my patches, instead of just suggesting that I should do
some research before sending them, because I already did.

Best regards
-- 
Íñigo Huguet