Subject: Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback_lock

From: Jakub Sitnicki <[email-masked]>
Date: Fri, 18 Nov 2022 18:50:43 +0100

On Fri, Nov 18, 2022 at 04:36 AM -08, Eric Dumazet wrote:
> On Fri, Nov 18, 2022 at 3:51 AM Tetsuo Handa
> <[email-masked]> 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/[email-masked]/T/
>
> Thanks.

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/[email-masked]/
[2] https://lore.kernel.org/netdev/[email-masked]
[3] https://lore.kernel.org/netdev/[email-masked]/

--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,
 		sock_release(sock);
 	else
 		sockfd_put(sock);
-
-	write_unlock(&sk->sk_callback_lock);
 err:
 	return ret;
 }