Date:   Wed, 15 Feb 2023 01:49:08 +0900
From:   Shigeru Yoshida <syoshida@redhat.com>
To:     Guillaume Nault <gnault@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+u7hGIAxhvyDG/2@kernel-devel>
References: <20230212162623.2301597-1-syoshida@redhat.com>
 <Y+pPXOqfrYkXPg1K@debian>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y+pPXOqfrYkXPg1K@debian>
Precedence: bulk

Hi Guillaume,

On Mon, Feb 13, 2023 at 03:55:24PM +0100, Guillaume Nault wrote:
> 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.

Thank you so much for your comment.  I got it.

> > +					       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.

Got it.

> > -	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.

Thanks for the helpful advice.

Just one more thing.  I created this patch based on the mainline linux
tree, but networking subsystem has own tree, net.  Is it preferable to
create a patch based on net tree for networking patches?

Thanks,
Shigeru

>