Re: WARNING: locking bug in inet_autobind

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Sep 19, 2022 at 12:52:45AM +0900, Tetsuo Handa wrote:
> syzbot is reporting locking bug in inet_autobind(), for
> commit 37159ef2c1ae1e69 ("l2tp: fix a lockdep splat") started
> calling 
> 
>   lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, "l2tp_sock")
> 
> in l2tp_tunnel_create() (which is currently in l2tp_tunnel_register()).
> How can we fix this problem?
> 

Just a theory, it seems that we have a memory corruption happened for
lockdep_set_class_and_name(), in l2tp_tunnel_register(), the "sk" gets
published before lockdep_set_class_and_name():

	tunnel->sock = sk;
	...
	lockdep_set_class_and_name(&sk->sk_lock.slock,...);

And what could happen is that sock_lock_init() races with the
l2tp_tunnel_register(), which results into two
lockdep_set_class_and_name()s race with each other.

Anyway, "sk" should not be published until its lock gets properly
initialized, could you try the following (untested)? Looks to me all
other code around the lockdep_set_class_and_name() should be moved
upwards, but I don't want to pretend I'm an expert ;-)

Regards,
Boqun

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7499c51b1850..1a01d23abc53 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1480,7 +1480,9 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

        sk = sock->sk;
        sock_hold(sk);
-       tunnel->sock = sk;
+       lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class,
+                                  "l2tp_sock");
+       smp_store_release(&tunnel->sock, sk);

        spin_lock_bh(&pn->l2tp_tunnel_list_lock);
        list_for_each_entry(tunnel_walk, &pn->l2tp_tunnel_list, list) {
@@ -1509,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

        tunnel->old_sk_destruct = sk->sk_destruct;
        sk->sk_destruct = &l2tp_tunnel_destruct;
-       lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class,
-                                  "l2tp_sock");
        sk->sk_allocation = GFP_ATOMIC;

        trace_register_tunnel(tunnel);  

>   ------------[ cut here ]------------
>   class->name=slock-AF_INET6 lock->name=l2tp_sock lock->key=l2tp_socket_class
>   WARNING: CPU: 2 PID: 9237 at kernel/locking/lockdep.c:940 look_up_lock_class+0xcc/0x140
>   Modules linked in:
>   CPU: 2 PID: 9237 Comm: a.out Not tainted 6.0.0-rc5-00094-ga335366bad13-dirty #860
>   Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
>   RIP: 0010:look_up_lock_class+0xcc/0x140
> 
> On 2019/05/16 14:46, syzbot wrote:
> > HEAD commit:    35c99ffa Merge tag 'for_linus' of git://git.kernel.org/pub..
> > git tree:       net-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=10e970f4a00000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=82f0809e8f0a8c87
> > dashboard link: https://syzkaller.appspot.com/bug?extid=94cc2a66fc228b23f360
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> 
> C reproducer is available at
> https://syzkaller.appspot.com/text?tag=ReproC&x=15062310080000 .
>