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@xxxxxxxxxxxxxx ,
> 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.
>
