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.
