[syzbot] BUG: sleeping function called from invalid context in static_key_slow_inc

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 064bc7312bd0 netdevsim: Fix memory leak of nsim_dev->fa_co..
git tree: net
console output: https://syzkaller.appspot.com/x/log.txt?x=13d3204e880000
kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/0634e1c0e4cb/disk-064bc731.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/fe1039d2de22/vmlinux-064bc731.xz
kernel image: https://storage.googleapis.com/syzbot-assets/5a0d673875fa/bzImage-064bc731.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+703d9e...@syzkaller.appspotmail.com

BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 9420, name: syz-executor.5
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
Preemption disabled at:
[<0000000000000000>] 0x0
CPU: 1 PID: 9420 Comm: syz-executor.5 Not tainted 6.1.0-rc4-syzkaller-00212-g064bc7312bd0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd1/0x138 lib/dump_stack.c:106
__might_resched.cold+0x222/0x26b kernel/sched/core.c:9890
percpu_down_read include/linux/percpu-rwsem.h:49 [inline]
cpus_read_lock+0x1b/0x140 kernel/cpu.c:310
static_key_slow_inc+0x12/0x20 kernel/jump_label.c:158
udp_tunnel_encap_enable include/net/udp_tunnel.h:187 [inline]
setup_udp_tunnel_sock+0x43d/0x550 net/ipv4/udp_tunnel_core.c:81
l2tp_tunnel_register+0xc51/0x1210 net/l2tp/l2tp_core.c:1509
pppol2tp_connect+0xcdc/0x1a10 net/l2tp/l2tp_ppp.c:723
__sys_connect_file+0x153/0x1a0 net/socket.c:1976
__sys_connect+0x165/0x1a0 net/socket.c:1993
__do_sys_connect net/socket.c:2003 [inline]
__se_sys_connect net/socket.c:2000 [inline]
__x64_sys_connect+0x73/0xb0 net/socket.c:2000
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fd94d28b639
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fd94e038168 EFLAGS: 00000246
ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 00007fd94d3abf80 RCX: 00007fd94d28b639
RDX: 000000000000002e RSI: 0000000020000000 RDI: 0000000000000003
RBP: 00007fd94d2e6ae9 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffde93128bf R14: 00007fd94e038300 R15: 0000000000022000
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit: 064bc7312bd0 netdevsim: Fix memory leak of nsim_dev->fa_co..
git tree: net

console+strace: https://syzkaller.appspot.com/x/log.txt?x=16b2b231880000

kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13cd2f79880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=109e1695880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/0634e1c0e4cb/disk-064bc731.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/fe1039d2de22/vmlinux-064bc731.xz
kernel image: https://storage.googleapis.com/syzbot-assets/5a0d673875fa/bzImage-064bc731.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+703d9e...@syzkaller.appspotmail.com

BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49

in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 3634, name: syz-executor167

preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0

3 locks held by syz-executor167/3634:
#0: ffffffff8df6b530 (cb_lock){++++}-{3:3}, at: genl_rcv+0x19/0x40 net/netlink/genetlink.c:860
#1: ffffffff8df6b5e8 (genl_mutex){+.+.}-{3:3}, at: genl_lock net/netlink/genetlink.c:33 [inline]
#1: ffffffff8df6b5e8 (genl_mutex){+.+.}-{3:3}, at: genl_rcv_msg+0x50d/0x780 net/netlink/genetlink.c:848
#2: ffff8880182fa0b8 (k-clock-AF_INET){+++.}-{2:2}, at: l2tp_tunnel_register+0x126/0x1210 net/l2tp/l2tp_core.c:1477

Preemption disabled at:
[<0000000000000000>] 0x0

CPU: 1 PID: 3634 Comm: syz-executor167 Not tainted 6.1.0-rc4-syzkaller-00212-g064bc7312bd0 #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd1/0x138 lib/dump_stack.c:106
__might_resched.cold+0x222/0x26b kernel/sched/core.c:9890
percpu_down_read include/linux/percpu-rwsem.h:49 [inline]
cpus_read_lock+0x1b/0x140 kernel/cpu.c:310
static_key_slow_inc+0x12/0x20 kernel/jump_label.c:158

udp_tunnel_encap_enable include/net/udp_tunnel.h:189 [inline]
setup_udp_tunnel_sock+0x3e1/0x550 net/ipv4/udp_tunnel_core.c:81
l2tp_tunnel_register+0xc51/0x1210 net/l2tp/l2tp_core.c:1509
l2tp_nl_cmd_tunnel_create+0x3d6/0x8b0 net/l2tp/l2tp_netlink.c:245
genl_family_rcv_msg_doit+0x228/0x320 net/netlink/genetlink.c:756
genl_family_rcv_msg net/netlink/genetlink.c:833 [inline]
genl_rcv_msg+0x445/0x780 net/netlink/genetlink.c:850
netlink_rcv_skb+0x157/0x430 net/netlink/af_netlink.c:2540
genl_rcv+0x28/0x40 net/netlink/genetlink.c:861
netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
netlink_unicast+0x547/0x7f0 net/netlink/af_netlink.c:1345
netlink_sendmsg+0x91b/0xe10 net/netlink/af_netlink.c:1921
sock_sendmsg_nosec net/socket.c:714 [inline]
sock_sendmsg+0xd3/0x120 net/socket.c:734
sock_no_sendpage+0x10c/0x160 net/core/sock.c:3219
kernel_sendpage.part.0+0x1d5/0x700 net/socket.c:3561
kernel_sendpage net/socket.c:3558 [inline]
sock_sendpage+0xe3/0x140 net/socket.c:1054
pipe_to_sendpage+0x2b1/0x380 fs/splice.c:361
splice_from_pipe_feed fs/splice.c:415 [inline]
__splice_from_pipe+0x449/0x8a0 fs/splice.c:559
splice_from_pipe fs/splice.c:594 [inline]
generic_splice_sendpage+0xd8/0x140 fs/splice.c:743
do_splice_from fs/splice.c:764 [inline]
direct_splice_actor+0x114/0x180 fs/splice.c:931
splice_direct_to_actor+0x335/0x8a0 fs/splice.c:886
do_splice_direct+0x1ab/0x280 fs/splice.c:974
do_sendfile+0xb19/0x1270 fs/read_write.c:1255
__do_sys_sendfile64 fs/read_write.c:1323 [inline]
__se_sys_sendfile64 fs/read_write.c:1309 [inline]
__x64_sys_sendfile64+0x1d0/0x210 fs/read_write.c:1309

do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

RIP: 0033:0x7f93d1567cb9
Code: 28 c3 e8 5a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffdd8ae4a88 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f93d1567cb9
RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000005
RBP: 00007f93d152b680 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000100000000 R11: 0000000000000246 R12: 00007f93d152b710
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>

[syzbot]

syzbot has bisected this issue to:

commit b68777d54fac21fc833ec26ea1a2a84f975ab035
Author: Jakub Sitnicki <ja...@cloudflare.com>
Date: Mon Nov 14 19:16:19 2022 +0000

l2tp: Serialize access to sk_user_data with sk_callback_lock

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1600bb49880000
start commit: 064bc7312bd0 netdevsim: Fix memory leak of nsim_dev->fa_co..
git tree: net
final oops: https://syzkaller.appspot.com/x/report.txt?x=1500bb49880000
console output: https://syzkaller.appspot.com/x/log.txt?x=1100bb49880000

kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13cd2f79880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=109e1695880000

Reported-by: syzbot+703d9e...@syzkaller.appspotmail.com
Fixes: b68777d54fac ("l2tp: Serialize access to sk_user_data with sk_callback_lock")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Hillf Danton]

On 17 Nov 2022 04:03:31 -0800

> syzbot has found a reproducer for the following issue on:
>
> HEAD commit: 064bc7312bd0 netdevsim: Fix memory leak of nsim_dev->fa_co..
> git tree: net
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=16b2b231880000
> kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
> dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13cd2f79880000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=109e1695880000

Set up udp tunnel without sk_callback_lock held.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git 064bc7312bd0

--- x/net/l2tp/l2tp_core.c
+++ l/net/l2tp/l2tp_core.c
@@ -1474,7 +1474,6 @@ int l2tp_tunnel_register(struct l2tp_tun
}

sk = sock->sk;
- write_lock(&sk->sk_callback_lock);

ret = l2tp_validate_socket(sk, net, tunnel->encap);
if (ret < 0)
@@ -1507,12 +1506,15 @@ int l2tp_tunnel_register(struct l2tp_tun
};

setup_udp_tunnel_sock(net, sock, &udp_cfg);
+ write_lock(&sk->sk_callback_lock);
} else {
+ write_lock(&sk->sk_callback_lock);
rcu_assign_sk_user_data(sk, tunnel);
}

tunnel->old_sk_destruct = sk->sk_destruct;
sk->sk_destruct = &l2tp_tunnel_destruct;
+ write_unlock(&sk->sk_callback_lock);
lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class,
"l2tp_sock");
sk->sk_allocation = GFP_ATOMIC;
@@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tun
if (tunnel->fd >= 0)
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
return 0;

err_sock:
@@ -1531,7 +1532,6 @@ err_sock:
else
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
err:
return ret;
}
--

[Tetsuo Handa]

syzbot is reporting sleep in atomic context at l2tp_tunnel_register() [1],
for commit b68777d54fac ("l2tp: Serialize access to sk_user_data with
sk_callback_lock") missed that udp_tunnel_encap_enable() from
setup_udp_tunnel_sock() might sleep.

Since we don't want to drop sk->sk_callback_lock inside
setup_udp_tunnel_sock() right before calling udp_tunnel_encap_enable(),
introduce a variant which does not call udp_tunnel_encap_enable(). And
call udp_tunnel_encap_enable() after dropping sk->sk_callback_lock.

Also, drop sk->sk_callback_lock before calling sock_release() in order to
avoid circular locking dependency problem.

Link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261 [1]
Reported-by: syzbot <syzbot+703d9e...@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>

Fixes: b68777d54fac ("l2tp: Serialize access to sk_user_data with sk_callback_lock")

---
F.Y.I. Below is the lockdep message:

======================================================
WARNING: possible circular locking dependency detected
6.1.0-rc5+ #2 Not tainted
------------------------------------------------------
a.out/2794 is trying to acquire lock:
ffff8c628878bdf0 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: sk_common_release+0x19/0xe0

but task is already holding lock:
ffff8c628878c078 (k-clock-AF_INET){+++.}-{2:2}, at: l2tp_tunnel_register+0x64/0x5e0 [l2tp_core]

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (k-clock-AF_INET){+++.}-{2:2}:
lock_acquire+0xc7/0x2e0
_raw_read_lock_bh+0x3d/0x80
sock_i_uid+0x19/0x40
udp_lib_lport_inuse+0x2c/0x120
udp_lib_get_port+0xf8/0x570
udp_v4_get_port+0xbb/0xc0
__inet_bind+0x10e/0x240
inet_bind+0x2b/0x40
kernel_bind+0xb/0x10
udp_sock_create4+0x97/0x160 [udp_tunnel]
l2tp_tunnel_sock_create+0x316/0x330 [l2tp_core]
l2tp_tunnel_register+0x394/0x5e0 [l2tp_core]
l2tp_nl_cmd_tunnel_create+0xe8/0x200 [l2tp_netlink]
genl_family_rcv_msg_doit.isra.17+0x102/0x140
genl_rcv_msg+0x112/0x270
netlink_rcv_skb+0x4f/0x100
genl_rcv+0x23/0x40
netlink_unicast+0x1a5/0x280
netlink_sendmsg+0x22f/0x490
sock_sendmsg+0x2e/0x40
____sys_sendmsg+0x1e9/0x210
___sys_sendmsg+0x77/0xb0
__sys_sendmsg+0x60/0xb0
__x64_sys_sendmsg+0x1a/0x20
do_syscall_64+0x34/0x80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

-> #1 (&table->hash[i].lock){+...}-{2:2}:
lock_acquire+0xc7/0x2e0
_raw_spin_lock_bh+0x31/0x40
udp_lib_get_port+0xda/0x570
udp_v4_get_port+0xbb/0xc0
__inet_bind+0x10e/0x240
inet_bind+0x2b/0x40
kernel_bind+0xb/0x10
udp_sock_create4+0x97/0x160 [udp_tunnel]
l2tp_tunnel_sock_create+0x316/0x330 [l2tp_core]
l2tp_tunnel_register+0x394/0x5e0 [l2tp_core]
l2tp_nl_cmd_tunnel_create+0xe8/0x200 [l2tp_netlink]
genl_family_rcv_msg_doit.isra.17+0x102/0x140
genl_rcv_msg+0x112/0x270
netlink_rcv_skb+0x4f/0x100
genl_rcv+0x23/0x40
netlink_unicast+0x1a5/0x280
netlink_sendmsg+0x22f/0x490
sock_sendmsg+0x2e/0x40
____sys_sendmsg+0x1e9/0x210
___sys_sendmsg+0x77/0xb0
__sys_sendmsg+0x60/0xb0
__x64_sys_sendmsg+0x1a/0x20
do_syscall_64+0x34/0x80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

-> #0 (k-sk_lock-AF_INET){+.+.}-{0:0}:
check_prevs_add+0x16a/0x1070
__lock_acquire+0x11bd/0x1670
lock_acquire+0xc7/0x2e0
udp_destroy_sock+0x2d/0xd0
sk_common_release+0x19/0xe0
udp_lib_close+0x9/0x10
inet_release+0x2e/0x60
__sock_release+0x7e/0xa0
sock_release+0xb/0x10
l2tp_tunnel_register+0x3f1/0x5e0 [l2tp_core]
l2tp_nl_cmd_tunnel_create+0xe8/0x200 [l2tp_netlink]
genl_family_rcv_msg_doit.isra.17+0x102/0x140
genl_rcv_msg+0x112/0x270
netlink_rcv_skb+0x4f/0x100
genl_rcv+0x23/0x40
netlink_unicast+0x1a5/0x280
netlink_sendmsg+0x22f/0x490
sock_sendmsg+0x2e/0x40
____sys_sendmsg+0x1e9/0x210
___sys_sendmsg+0x77/0xb0
__sys_sendmsg+0x60/0xb0
__x64_sys_sendmsg+0x1a/0x20
do_syscall_64+0x34/0x80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

other info that might help us debug this:

Chain exists of:
k-sk_lock-AF_INET --> &table->hash[i].lock --> k-clock-AF_INET

Possible unsafe locking scenario:

CPU0 CPU1
---- ----
lock(k-clock-AF_INET);
lock(&table->hash[i].lock);
lock(k-clock-AF_INET);
lock(k-sk_lock-AF_INET);

*** DEADLOCK ***

3 locks held by a.out/2794:
#0: ffffffffb466fc30 (cb_lock){++++}-{3:3}, at: genl_rcv+0x14/0x40
#1: ffffffffb466fcc8 (genl_mutex){+.+.}-{3:3}, at: genl_rcv_msg+0x14d/0x270
#2: ffff8c628878c078 (k-clock-AF_INET){+++.}-{2:2}, at: l2tp_tunnel_register+0x64/0x5e0 [l2tp_core]

include/net/udp_tunnel.h | 2 ++
net/ipv4/udp_tunnel_core.c | 10 ++++++++--
net/l2tp/l2tp_core.c | 10 +++++-----
3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index 72394f441dad..a84fa57bc750 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -92,6 +92,8 @@ struct udp_tunnel_sock_cfg {
/* Setup the given (UDP) sock to receive UDP encapsulated packets */
void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
struct udp_tunnel_sock_cfg *sock_cfg);
+void setup_udp_tunnel_sock_no_enable(struct net *net, struct socket *sock,
+ struct udp_tunnel_sock_cfg *sock_cfg);

/* -- List of parsable UDP tunnel types --
*
diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
index 8242c8947340..dff825664000 100644
--- a/net/ipv4/udp_tunnel_core.c
+++ b/net/ipv4/udp_tunnel_core.c
@@ -57,8 +57,8 @@ int udp_sock_create4(struct net *net, struct udp_port_cfg *cfg,
}
EXPORT_SYMBOL(udp_sock_create4);

-void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
- struct udp_tunnel_sock_cfg *cfg)
+void setup_udp_tunnel_sock_no_enable(struct net *net, struct socket *sock,
+ struct udp_tunnel_sock_cfg *cfg)
{
struct sock *sk = sock->sk;

@@ -77,7 +77,13 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
udp_sk(sk)->encap_destroy = cfg->encap_destroy;
udp_sk(sk)->gro_receive = cfg->gro_receive;
udp_sk(sk)->gro_complete = cfg->gro_complete;
+}
+EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock_no_enable);

+void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
+ struct udp_tunnel_sock_cfg *cfg)
+{
+ setup_udp_tunnel_sock_no_enable(net, sock, cfg);
udp_tunnel_encap_enable(sock);
}
EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 754fdda8a5f5..a4f611196c83 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1506,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
.encap_destroy = l2tp_udp_encap_destroy,
};

- setup_udp_tunnel_sock(net, sock, &udp_cfg);
+ setup_udp_tunnel_sock_no_enable(net, sock, &udp_cfg);
} else {
rcu_assign_sk_user_data(sk, tunnel);
}
@@ -1519,19 +1519,19 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

trace_register_tunnel(tunnel);

+ write_unlock(&sk->sk_callback_lock);
+ if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
+ udp_tunnel_encap_enable(sock);

if (tunnel->fd >= 0)
sockfd_put(sock);
-
- write_unlock(&sk->sk_callback_lock);
return 0;

err_sock:

+ write_unlock(&sk->sk_callback_lock);
if (tunnel->fd < 0)
sock_release(sock);
else
sockfd_put(sock);
-

- write_unlock(&sk->sk_callback_lock);
err:
return ret;
}
--

2.18.4

[Eric Dumazet]

On Fri, Nov 18, 2022 at 3:51 AM Tetsuo Handa
<penguin...@i-love.sakura.ne.jp> wrote:
>
> syzbot is reporting sleep in atomic context at l2tp_tunnel_register() [1],
> for commit b68777d54fac ("l2tp: Serialize access to sk_user_data with
> sk_callback_lock") missed that udp_tunnel_encap_enable() from
> setup_udp_tunnel_sock() might sleep.
>
> Since we don't want to drop sk->sk_callback_lock inside
> setup_udp_tunnel_sock() right before calling udp_tunnel_encap_enable(),
> introduce a variant which does not call udp_tunnel_encap_enable(). And
> call udp_tunnel_encap_enable() after dropping sk->sk_callback_lock.
>
> Also, drop sk->sk_callback_lock before calling sock_release() in order to
> avoid circular locking dependency problem.

Please look at recent discussion, your patch does not address another
fundamental problem.

Also, Jakub was working on a fix already. Perhaps sync with him to
avoid duplicate work.

https://lore.kernel.org/netdev/20221114191619...@cloudflare.com/T/

Thanks.

[Tetsuo Handa]

On 2022/11/18 21:36, Eric Dumazet wrote:
> Please look at recent discussion, your patch does not address another
> fundamental problem.
>
> Also, Jakub was working on a fix already. Perhaps sync with him to
> avoid duplicate work.

I can't afford monitoring all mailing lists. Since a thread at syzkaller-bugs group
did not get that information, I started this work. Please consider including
syzbot+XXXXXX...@syzkaller.appspotmail.com into the discussions so that
we can google for recent discussions (if any) using mail address as a keyword.

>
> https://lore.kernel.org/netdev/20221114191619...@cloudflare.com/T/
>
> Thanks.

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
inconsistent lock state in l2tp_tunnel_destruct

================================
WARNING: inconsistent lock state
6.1.0-rc4-syzkaller-00212-g064bc7312bd0-dirty #0 Not tainted
--------------------------------
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
swapper/0/0 [HC0[0]:SC1[3]:HE1:SE0] takes:
ffff888145ea44f8 (k-clock-AF_INET){++?.}-{2:2}, at: l2tp_tunnel_destruct+0xe6/0x2d0 net/l2tp/l2tp_core.c:1153
{SOFTIRQ-ON-W} state was registered at:
lock_acquire kernel/locking/lockdep.c:5668 [inline]
lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633
__raw_write_lock include/linux/rwlock_api_smp.h:209 [inline]
_raw_write_lock+0x2e/0x40 kernel/locking/spinlock.c:300
l2tp_tunnel_register+0xc61/0x1200 net/l2tp/l2tp_core.c:1509

l2tp_nl_cmd_tunnel_create+0x3d6/0x8b0 net/l2tp/l2tp_netlink.c:245
genl_family_rcv_msg_doit+0x228/0x320 net/netlink/genetlink.c:756
genl_family_rcv_msg net/netlink/genetlink.c:833 [inline]
genl_rcv_msg+0x445/0x780 net/netlink/genetlink.c:850
netlink_rcv_skb+0x157/0x430 net/netlink/af_netlink.c:2540
genl_rcv+0x28/0x40 net/netlink/genetlink.c:861
netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
netlink_unicast+0x547/0x7f0 net/netlink/af_netlink.c:1345
netlink_sendmsg+0x91b/0xe10 net/netlink/af_netlink.c:1921
sock_sendmsg_nosec net/socket.c:714 [inline]
sock_sendmsg+0xd3/0x120 net/socket.c:734

____sys_sendmsg+0x712/0x8c0 net/socket.c:2482
___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
__sys_sendmsg+0xf7/0x1c0 net/socket.c:2565

do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

irq event stamp: 5256464
hardirqs last enabled at (5256464): [<ffffffff89f47d84>] __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:151 [inline]
hardirqs last enabled at (5256464): [<ffffffff89f47d84>] _raw_spin_unlock_irqrestore+0x54/0x70 kernel/locking/spinlock.c:194
hardirqs last disabled at (5256463): [<ffffffff89f47b12>] __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:108 [inline]
hardirqs last disabled at (5256463): [<ffffffff89f47b12>] _raw_spin_lock_irqsave+0x52/0x60 kernel/locking/spinlock.c:162
softirqs last enabled at (5256370): [<ffffffff814c1d03>] invoke_softirq kernel/softirq.c:445 [inline]
softirqs last enabled at (5256370): [<ffffffff814c1d03>] __irq_exit_rcu+0x123/0x180 kernel/softirq.c:650
softirqs last disabled at (5256383): [<ffffffff814c1d03>] invoke_softirq kernel/softirq.c:445 [inline]
softirqs last disabled at (5256383): [<ffffffff814c1d03>] __irq_exit_rcu+0x123/0x180 kernel/softirq.c:650

other info that might help us debug this:

Possible unsafe locking scenario:

CPU0

----
lock(k-clock-AF_INET);
<Interrupt>
lock(k-clock-AF_INET);

*** DEADLOCK ***

1 lock held by swapper/0/0:
#0: ffffffff8c58fc60 (rcu_callback){....}-{0:0}, at: rcu_do_batch kernel/rcu/tree.c:2239 [inline]
#0: ffffffff8c58fc60 (rcu_callback){....}-{0:0}, at: rcu_core+0x7ab/0x1980 kernel/rcu/tree.c:2510

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.1.0-rc4-syzkaller-00212-g064bc7312bd0-dirty #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Call Trace:

<IRQ>

__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd1/0x138 lib/dump_stack.c:106

print_usage_bug kernel/locking/lockdep.c:3963 [inline]
valid_state kernel/locking/lockdep.c:3975 [inline]
mark_lock_irq kernel/locking/lockdep.c:4178 [inline]
mark_lock.part.0.cold+0x18/0xd8 kernel/locking/lockdep.c:4634
mark_lock kernel/locking/lockdep.c:4598 [inline]
mark_usage kernel/locking/lockdep.c:4529 [inline]
__lock_acquire+0x11d9/0x56d0 kernel/locking/lockdep.c:5009
lock_acquire kernel/locking/lockdep.c:5668 [inline]
lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633
__raw_write_lock_bh include/linux/rwlock_api_smp.h:202 [inline]
_raw_write_lock_bh+0x33/0x40 kernel/locking/spinlock.c:334
l2tp_tunnel_destruct+0xe6/0x2d0 net/l2tp/l2tp_core.c:1153
__sk_destruct+0x51/0x710 net/core/sock.c:2122
rcu_do_batch kernel/rcu/tree.c:2250 [inline]
rcu_core+0x81f/0x1980 kernel/rcu/tree.c:2510
__do_softirq+0x1fb/0xadc kernel/softirq.c:571
invoke_softirq kernel/softirq.c:445 [inline]
__irq_exit_rcu+0x123/0x180 kernel/softirq.c:650
irq_exit_rcu+0x9/0x20 kernel/softirq.c:662
sysvec_apic_timer_interrupt+0x97/0xc0 arch/x86/kernel/apic/apic.c:1107
</IRQ>
<TASK>
asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:649
RIP: 0010:native_save_fl arch/x86/include/asm/irqflags.h:29 [inline]
RIP: 0010:arch_local_save_flags arch/x86/include/asm/irqflags.h:70 [inline]
RIP: 0010:arch_irqs_disabled arch/x86/include/asm/irqflags.h:130 [inline]
RIP: 0010:acpi_safe_halt drivers/acpi/processor_idle.c:113 [inline]
RIP: 0010:acpi_idle_do_entry+0x1fd/0x2a0 drivers/acpi/processor_idle.c:572
Code: 89 de e8 66 ac 8b f7 84 db 75 ac e8 ed af 8b f7 e8 78 30 92 f7 eb 0c e8 e1 af 8b f7 0f 00 2d 7a 63 be 00 e8 d5 af 8b f7 fb f4 <9c> 5b 81 e3 00 02 00 00 fa 31 ff 48 89 de e8 90 ac 8b f7 48 85 db
RSP: 0018:ffffffff8c207d28 EFLAGS: 00000293
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffffffff8c2bc9c0 RSI: ffffffff89f469ab RDI: 0000000000000000
RBP: ffff888145cc0064 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
R13: ffff888145cc0000 R14: ffff888145cc0064 R15: ffff888145ebe804
acpi_idle_enter+0x368/0x510 drivers/acpi/processor_idle.c:709
cpuidle_enter_state+0x1af/0xd40 drivers/cpuidle/cpuidle.c:239
cpuidle_enter+0x4e/0xa0 drivers/cpuidle/cpuidle.c:356
call_cpuidle kernel/sched/idle.c:155 [inline]
cpuidle_idle_call kernel/sched/idle.c:236 [inline]
do_idle+0x3f7/0x590 kernel/sched/idle.c:303
cpu_startup_entry+0x18/0x20 kernel/sched/idle.c:400
rest_init+0x16d/0x270 init/main.c:729
arch_call_rest_init+0x13/0x1c init/main.c:890
start_kernel+0x477/0x498 init/main.c:1145
secondary_startup_64_no_verify+0xce/0xdb
</TASK>
----------------
Code disassembly (best guess):
0: 89 de mov %ebx,%esi
2: e8 66 ac 8b f7 callq 0xf78bac6d
7: 84 db test %bl,%bl
9: 75 ac jne 0xffffffb7
b: e8 ed af 8b f7 callq 0xf78baffd
10: e8 78 30 92 f7 callq 0xf792308d
15: eb 0c jmp 0x23
17: e8 e1 af 8b f7 callq 0xf78baffd
1c: 0f 00 2d 7a 63 be 00 verw 0xbe637a(%rip) # 0xbe639d
23: e8 d5 af 8b f7 callq 0xf78baffd
28: fb sti
29: f4 hlt
* 2a: 9c pushfq <-- trapping instruction
2b: 5b pop %rbx
2c: 81 e3 00 02 00 00 and $0x200,%ebx
32: fa cli
33: 31 ff xor %edi,%edi
35: 48 89 de mov %rbx,%rsi
38: e8 90 ac 8b f7 callq 0xf78baccd
3d: 48 85 db test %rbx,%rbx


Tested on:

commit: 064bc731 netdevsim: Fix memory leak of nsim_dev->fa_co..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
console output: https://syzkaller.appspot.com/x/log.txt?x=148c924e880000

kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=110210f9880000

[Eric Dumazet]

On Fri, Nov 18, 2022 at 5:19 AM Tetsuo Handa
<penguin...@i-love.sakura.ne.jp> wrote:
>
> On 2022/11/18 21:36, Eric Dumazet wrote:
> > Please look at recent discussion, your patch does not address another
> > fundamental problem.
> >
> > Also, Jakub was working on a fix already. Perhaps sync with him to
> > avoid duplicate work.
>
> I can't afford monitoring all mailing lists. Since a thread at syzkaller-bugs group
> did not get that information, I started this work. Please consider including
> syzbot+XXXXXX...@syzkaller.appspotmail.com into the discussions so that
> we can google for recent discussions (if any) using mail address as a keyword.
>

This is not going to happen.

The discussion happened before the reports were made public.

No more than 7 syzbot reports are attached to the same root cause.

We deal with hundreds of syzbot reports per week, there is no way we
can retroactively find all relevant netdev@ threads.

If you can not afford making sure you are not wasting your time, this
is your call.

[Jakub Sitnicki]

Thanks for the patch, Tetsuo.

As Eric has pointed out [1], there is another problem - in addition to
sleeping in atomic context, I have also failed to use the write_lock
variant which disabled BH locally.

The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
because we grab sk_callback_lock in softirq context, which can then
block waiting on us if:

1) it runs on the same CPU, or

CPU0
----
lock(clock-AF_INET6);
<Interrupt>
lock(clock-AF_INET6);

2) lock ordering leads to priority inversion

CPU0 CPU1
---- ----
lock(clock-AF_INET6);
local_irq_disable();
lock(&tcp_hashinfo.bhash[i].lock);
lock(clock-AF_INET6);
<Interrupt>
lock(&tcp_hashinfo.bhash[i].lock);

IOW, your patch works if we also s/write_\(un\)\?lock/write_\1lock_bh/.

But, I also have an alternative idea - instead of pulling the function
call that might sleep out of the critical section, I think we can make
the critical section much shorter by rearranging the tunnel
initialization code slightly. That is, a change like below.

-jkbs

[1] https://lore.kernel.org/netdev/CANn89iLQUZnyGNCn2GpW31FX...@mail.gmail.com/
[2] https://lore.kernel.org/netdev/000000000000e3...@google.com
[3] https://lore.kernel.org/netdev/000000000000df...@google.com/


--8<--

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 754fdda8a5f5..07454c0418e3 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1474,11 +1474,15 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

}

sk = sock->sk;
- write_lock(&sk->sk_callback_lock);

+ write_lock_bh(&sk->sk_callback_lock);

ret = l2tp_validate_socket(sk, net, tunnel->encap);
if (ret < 0)

goto err_sock;
+ if (tunnel->encap != L2TP_ENCAPTYPE_UDP)
+ rcu_assign_sk_user_data(sk, tunnel);
+
+ write_unlock_bh(&sk->sk_callback_lock);

tunnel->l2tp_net = net;
pn = l2tp_pernet(net);
@@ -1507,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
};

setup_udp_tunnel_sock(net, sock, &udp_cfg);
- } else {
- rcu_assign_sk_user_data(sk, tunnel);

}

tunnel->old_sk_destruct = sk->sk_destruct;

@@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

if (tunnel->fd >= 0)
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
return 0;

err_sock:

@@ -1530,8 +1531,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

[Jakub Sitnicki]

On Fri, Nov 18, 2022 at 06:50 PM +01, Jakub Sitnicki wrote:

[...]

> But, I also have an alternative idea - instead of pulling the function

> call that might sleep out of the critical section, I think we could make

> the critical section much shorter by rearranging the tunnel
> initialization code slightly. That is, a change like below.

[...]

> --8<--
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 754fdda8a5f5..07454c0418e3 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c

> @@ -1474,11 +1474,15 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> }
>
> sk = sock->sk;
> - write_lock(&sk->sk_callback_lock);
> + write_lock_bh(&sk->sk_callback_lock);
>
> ret = l2tp_validate_socket(sk, net, tunnel->encap);
> if (ret < 0)
> goto err_sock;
> + if (tunnel->encap != L2TP_ENCAPTYPE_UDP)
> + rcu_assign_sk_user_data(sk, tunnel);

sk_user_data needs to be reset back to NULL if we bail out when the
tunnel already exists. Will add that and turn it into a patch tomorrow.

> +
> + write_unlock_bh(&sk->sk_callback_lock);
>
> tunnel->l2tp_net = net;
> pn = l2tp_pernet(net);
> @@ -1507,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> };
>
> setup_udp_tunnel_sock(net, sock, &udp_cfg);
> - } else {
> - rcu_assign_sk_user_data(sk, tunnel);
> }
>
> tunnel->old_sk_destruct = sk->sk_destruct;

> @@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

> if (tunnel->fd >= 0)
> sockfd_put(sock);
>
> - write_unlock(&sk->sk_callback_lock);
> return 0;
>
> err_sock:

> @@ -1530,8 +1531,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

[Hillf Danton]

On 17 Nov 2022 04:03:31 -0800

> syzbot has found a reproducer for the following issue on:
>
> HEAD commit: 064bc7312bd0 netdevsim: Fix memory leak of nsim_dev->fa_co..
> git tree: net
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=16b2b231880000
> kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
> dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13cd2f79880000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=109e1695880000

Set up udp tunnel without sk_callback_lock held.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git 064bc7312bd0

--- x/net/l2tp/l2tp_core.c
+++ l/net/l2tp/l2tp_core.c
@@ -1474,7 +1474,6 @@ int l2tp_tunnel_register(struct l2tp_tun
}

sk = sock->sk;
- write_lock(&sk->sk_callback_lock);

ret = l2tp_validate_socket(sk, net, tunnel->encap);
if (ret < 0)

@@ -1507,12 +1506,15 @@ int l2tp_tunnel_register(struct l2tp_tun
};

setup_udp_tunnel_sock(net, sock, &udp_cfg);

+ write_lock_bh(&sk->sk_callback_lock);
} else {
+ write_lock_bh(&sk->sk_callback_lock);

rcu_assign_sk_user_data(sk, tunnel);
}

tunnel->old_sk_destruct = sk->sk_destruct;

sk->sk_destruct = &l2tp_tunnel_destruct;
+ write_unlock_bh(&sk->sk_callback_lock);

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

@@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tun

if (tunnel->fd >= 0)
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
return 0;

err_sock:

@@ -1531,7 +1532,6 @@ err_sock:
else
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
err:
return ret;
}

--

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+703d9e...@syzkaller.appspotmail.com

Tested on:

commit: 064bc731 netdevsim: Fix memory leak of nsim_dev->fa_co..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

console output: https://syzkaller.appspot.com/x/log.txt?x=12a7010d880000

kernel config: https://syzkaller.appspot.com/x/.config?x=a33ac7bbc22a8c35
dashboard link: https://syzkaller.appspot.com/bug?extid=703d9e154b3b58277261
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=11425701880000

Note: testing is done by a robot and is best-effort only.

[Tetsuo Handa]

On 2022/11/19 2:50, Jakub Sitnicki wrote:
> Thanks for the patch, Tetsuo.
>
> As Eric has pointed out [1], there is another problem - in addition to
> sleeping in atomic context, I have also failed to use the write_lock
> variant which disabled BH locally.
>
> The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
> because we grab sk_callback_lock in softirq context, which can then
> block waiting on us if:

Below is another approach I was thinking of, for reusing existing locks is prone
to locking bugs like [2] and [3].

I couldn't interpret "Write-protected by @sk_callback_lock." part because
it does not say what lock is needed for protecting sk_user_data for read access.

Is it possible to use a mutex dedicated for l2tp_tunnel_destruct() (and optionally
setup_udp_tunnel_sock_no_enable() in order not to create l2tp_tunnel_register_mutex =>
cpu_hotplug_lock chain) ?

By the way I haven't heard an response on

Since userspace-supplied file descriptor has to be a datagram socket,
can we somehow copy the source/destination addresses from
userspace-supplied socket to kernel-created socket?

at https://lkml.kernel.org/r/c9695548-3f27-dda1...@I-love.SAKURA.ne.jp
(that is, always create a new socket in order to be able to assign lockdep class
before that socket is used).

diff --git a/include/net/sock.h b/include/net/sock.h
index e0517ecc6531..49473013afa6 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -323,7 +323,7 @@ struct sk_filter;
* @sk_tskey: counter to disambiguate concurrent tstamp requests
* @sk_zckey: counter to order MSG_ZEROCOPY notifications
* @sk_socket: Identd and reporting IO signals
- * @sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock.
+ * @sk_user_data: RPC layer private data.
* @sk_frag: cached page frag
* @sk_peek_off: current peek_offset value
* @sk_send_head: front of stuff to transmit
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 754fdda8a5f5..2bfcf6968d89 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1150,10 +1150,8 @@ static void l2tp_tunnel_destruct(struct sock *sk)
}

/* Remove hooks into tunnel socket */
- write_lock_bh(&sk->sk_callback_lock);
sk->sk_destruct = tunnel->old_sk_destruct;
sk->sk_user_data = NULL;
- write_unlock_bh(&sk->sk_callback_lock);

/* Call the original destructor */
if (sk->sk_destruct)
@@ -1455,6 +1453,7 @@ static int l2tp_validate_socket(const struct sock *sk, const struct net *net,

int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

struct l2tp_tunnel_cfg *cfg)
{
+ static DEFINE_MUTEX(l2tp_tunnel_register_mutex);
struct l2tp_tunnel *tunnel_walk;
struct l2tp_net *pn;
struct socket *sock;
@@ -1474,7 +1473,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

}

sk = sock->sk;
- write_lock(&sk->sk_callback_lock);

+ mutex_lock(&l2tp_tunnel_register_mutex);

ret = l2tp_validate_socket(sk, net, tunnel->encap);
if (ret < 0)

@@ -1519,19 +1518,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

trace_register_tunnel(tunnel);

+ mutex_unlock(&l2tp_tunnel_register_mutex);

if (tunnel->fd >= 0)
sockfd_put(sock);

- write_unlock(&sk->sk_callback_lock);
return 0;

err_sock:

+ mutex_unlock(&l2tp_tunnel_register_mutex);
if (tunnel->fd < 0)

[Jakub Sitnicki]

On Sat, Nov 19, 2022 at 07:08 PM +09, Tetsuo Handa wrote:
> On 2022/11/19 2:50, Jakub Sitnicki wrote:
>> Thanks for the patch, Tetsuo.
>>
>> As Eric has pointed out [1], there is another problem - in addition to
>> sleeping in atomic context, I have also failed to use the write_lock
>> variant which disabled BH locally.
>>
>> The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
>> because we grab sk_callback_lock in softirq context, which can then
>> block waiting on us if:
>
> Below is another approach I was thinking of, for reusing existing locks is prone
> to locking bugs like [2] and [3].
>
> I couldn't interpret "Write-protected by @sk_callback_lock." part because
> it does not say what lock is needed for protecting sk_user_data for read access.

sk_user_data is RCU-protected on reader-side. But we still need to
synchronize writers.

> Is it possible to use a mutex dedicated for l2tp_tunnel_destruct() (and optionally
> setup_udp_tunnel_sock_no_enable() in order not to create l2tp_tunnel_register_mutex =>
> cpu_hotplug_lock chain) ?

No, we need to a common lock to synchronize with other users in the net
stack (reuseport groups, sockmap/psock to name a couple).

> By the way I haven't heard an response on
>
> Since userspace-supplied file descriptor has to be a datagram socket,
> can we somehow copy the source/destination addresses from
> userspace-supplied socket to kernel-created socket?
>
> at https://lkml.kernel.org/r/c9695548-3f27-dda1...@I-love.SAKURA.ne.jp
> (that is, always create a new socket in order to be able to assign lockdep class
> before that socket is used).

This is a drive by fix for me to l2tp, so I might not be the best person
to ask, but I will take a look at the thread.

[...]