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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Jun 2021 20:02:52 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-can@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] can: bcm/raw/isotp: use per module netdevice notifier

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.