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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Fri, Nov 18, 2022 at 04:36 AM -08, Eric Dumazet wrote:
> On Fri, Nov 18, 2022 at 3:51 AM Tetsuo Handa
> <penguin-kernel@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.124659-1-jakub@cloudflare.com/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/CANn89iLQUZnyGNCn2GpW31FXpE_Lt7a5Urr21RqzfAE4sYxs+w@mail.gmail.com/
[2] https://lore.kernel.org/netdev/000000000000e38b6605eda76f98@google.com
[3] https://lore.kernel.org/netdev/000000000000dfa31e05eda76f75@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,
 		sock_release(sock);
 	else
 		sockfd_put(sock);
-
-	write_unlock(&sk->sk_callback_lock);
 err:
 	return ret;
 }