Date:   Mon, 13 Feb 2023 15:55:24 +0100
From:   Guillaume Nault <gnault@redhat.com>
To:     Shigeru Yoshida <syoshida@redhat.com>
Cc:     jchapman@katalix.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in
 l2tp_tunnel_register()
Message-ID: <Y+pPXOqfrYkXPg1K@debian>
References: <20230212162623.2301597-1-syoshida@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230212162623.2301597-1-syoshida@redhat.com>
Precedence: bulk

On Mon, Feb 13, 2023 at 01:26:23AM +0900, Shigeru Yoshida wrote:
> +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
> +					       struct l2tp_connect_info *info,

Please make "*info" const.

> +					       bool *new_tunnel)
> +{
> +	struct l2tp_tunnel *tunnel;
> +	int error;
> +
> +	*new_tunnel = false;
> +
> +	tunnel = l2tp_tunnel_get(net, info->tunnel_id);
> +
> +	/* Special case: create tunnel context if session_id and
> +	 * peer_session_id is 0. Otherwise look up tunnel using supplied
> +	 * tunnel id.
> +	 */
> +	if (!info->session_id && !info->peer_session_id) {
> +		if (!tunnel) {
> +			struct l2tp_tunnel_cfg tcfg = {
> +				.encap = L2TP_ENCAPTYPE_UDP,
> +			};
> +
> +			/* Prevent l2tp_tunnel_register() from trying to set up
> +			 * a kernel socket.
> +			 */
> +			if (info->fd < 0)
> +				return ERR_PTR(-EBADF);
> +
> +			error = l2tp_tunnel_create(info->fd,
> +						   info->version,
> +						   info->tunnel_id,
> +						   info->peer_tunnel_id, &tcfg,
> +						   &tunnel);
> +			if (error < 0)
> +				return ERR_PTR(error);
> +
> +			l2tp_tunnel_inc_refcount(tunnel);
> +			error = l2tp_tunnel_register(tunnel, net, &tcfg);
> +			if (error < 0) {
> +				kfree(tunnel);
> +				return ERR_PTR(error);
> +			}
> +
> +			*new_tunnel = true;
> +		}
> +	} else {
> +		/* Error if we can't find the tunnel */
> +		if (!tunnel)
> +			return ERR_PTR(-ENOENT);
> +
> +		/* Error if socket is not prepped */
> +		if (!tunnel->sock) {
> +			l2tp_tunnel_dec_refcount(tunnel);
> +			return ERR_PTR(-ENOENT);
> +		}
> +	}
> +
> +	return tunnel;
> +}
> +
>  /* connect() handler. Attach a PPPoX socket to a tunnel UDP socket
>   */
>  static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
> @@ -663,7 +722,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>  	struct pppol2tp_session *ps;
>  	struct l2tp_session_cfg cfg = { 0, };
>  	bool drop_refcnt = false;
> -	bool drop_tunnel = false;
>  	bool new_session = false;
>  	bool new_tunnel = false;
>  	int error;
> @@ -672,6 +730,10 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>  	if (error < 0)
>  		return error;
>  
> +	tunnel = pppol2tp_tunnel_get(sock_net(sk), &info, &new_tunnel);
> +	if (IS_ERR(tunnel))
> +		return PTR_ERR(tunnel);
> +
>  	lock_sock(sk);
>  
>  	/* Check for already bound sockets */
> @@ -689,57 +751,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>  	if (!info.tunnel_id)
>  		goto end;

The original code did test info.tunnel_id before trying to get or
create the tunnel (as it doesn't make sense to work on a tunnel whose
ID is 0). So we need move this test before the pppol2tp_tunnel_get()
call.

> -	tunnel = l2tp_tunnel_get(sock_net(sk), info.tunnel_id);
> -	if (tunnel)
> -		drop_tunnel = true;
> -
> -	/* Special case: create tunnel context if session_id and
> -	 * peer_session_id is 0. Otherwise look up tunnel using supplied
> -	 * tunnel id.
> -	 */

Just a note for your future submissions: for networking patches, we
normally indicate which tree the patch is targetted to in the mail
subject (for example "[PATCH net v2]"). Also, you should Cc:
the author of the patch listed in the Fixes tag.