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:

quoted

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,

quoted

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

quoted

-> 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.

quoted

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.

quoted

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.