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