netdev - Re: [PATCH] can: bcm/raw/isotp: use per module netdevice notifier

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]

Message-ID: <5e4693cf-4691-e7da-9a04-3e70cc449bf5@i-love.sakura.ne.jp>
Date:   Fri, 4 Jun 2021 09:22:52 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-can@...r.kernel.org
Subject: Re: [PATCH] can: bcm/raw/isotp: use per module netdevice notifier

Adding Kirill for commit 328fbe747ad4622f ("net: Close race between
{un, }register_netdevice_notifier() and setup_net()/cleanup_net()").

I'm proposing this patch because calling {,un}register_netdevice_notifier()
on every socket {initialization,destruction} is killing ability to
concurrently run cleanup_net() enough for khungtaskd to complain.

You are referring something with raw_init() in the above commit.
What is your concern? (I'm asking you in case this patch breaks
something you mentioned.)

On 2021/06/03 20:02, Tetsuo Handa wrote:
> On 2021/06/03 15:09, Oliver Hartkopp wrote:
>> so I wonder why only the *registering* of a netdev notifier can cause a 'hang' in that way?!?
> 
> Not only the *registering* of a netdev notifier causes a 'hang' in that way.
> For example,
> 
>> My assumption would be that a wrong type of locking mechanism is used in
>> register_netdevice_notifier() which you already tried to address here:
>>
>> https://syzkaller.appspot.com/bug?id=391b9498827788b3cc6830226d4ff5be87107c30
> 
> the result of
> 
>> -> https://syzkaller.appspot.com/text?tag=Patch&x=106ad8dbd00000
> 
> is https://syzkaller.appspot.com/text?tag=CrashReport&x=1705d92fd00000 which
> says that the *unregistering* of a netdev notifier caused a 'hang'. In other
> words, making register_netdevice_notifier() killable is not sufficient, and
> it is impossible to make unregister_netdevice_notifier() killable.
> 
> Moreover, there are modules (e.g. CAN driver's raw/bcm/isotp modules) which are
> not prepared for register_netdevice_notifier() failure. Therefore, I made this
> patch which did not cause a 'hang' even if "many things" (see the next paragraph)
> are run concurrently.
> 
>> The removal of one to three data structures in CAN is not time consuming.
> 
> Yes, it would be true that CAN socket's operations alone are not time consuming.
> But since syzkaller is a fuzzer, it concurrently runs many things (including
> non-CAN sockets operations and various networking devices), and cleanup_net()
> for some complicated combinations will be time consuming.
> 
>> IMHO we need to fix some locking semantics (with pernet_ops_rwsem??) here.
> 
> Assuming that lockdep is correctly detecting possibility of deadlock, no lockdep
> warning indicates that there is no locking semantics error here. In other words,
> taking locks (e.g. pernet_ops_rwsem, rtnl_mutex) that are shared by many protocols
> causes fast protocols to be slowed down to the possible slowest operations.
> 
> As explained at
> https://lkml.kernel.org/r/CACT4Y+Y8KmaoEj0L8g=wX4owS38mjNLVMMLsjyoN8DU9n=FrrQ@mail.gmail.com ,
> unbounded asynchronous queuing is always a recipe for disaster. cleanup_net() is
> called from a WQ context, and does time consuming operations with pernet_ops_rwsem
> held for read. Therefore, reducing frequency of holding pernet_ops_rwsem for write
> (because CAN driver's raw/bcm/isotp modules are calling {,un}register_netdevice_notifier()
> on every socket) helps cleanup_net() to make more progress; a low-hanging mitigation
> for this problem.
>