[PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Shigeru Yoshida]

When a file descriptor of pppol2tp socket is passed as file descriptor
of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
This situation is reproduced by the following program:

int main(void)
{
	int sock;
	struct sockaddr_pppol2tp addr;

	sock = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
	if (sock < 0) {
		perror("socket");
		return 1;
	}

	addr.sa_family = AF_PPPOX;
	addr.sa_protocol = PX_PROTO_OL2TP;
	addr.pppol2tp.pid = 0;
	addr.pppol2tp.fd = sock;
	addr.pppol2tp.addr.sin_family = PF_INET;
	addr.pppol2tp.addr.sin_port = htons(0);
	addr.pppol2tp.addr.sin_addr.s_addr = inet_addr("192.168.0.1");
	addr.pppol2tp.s_tunnel = 1;
	addr.pppol2tp.s_session = 0;
	addr.pppol2tp.d_tunnel = 0;
	addr.pppol2tp.d_session = 0;

	if (connect(sock, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
		perror("connect");
		return 1;
	}

	return 0;
}

This program causes the following lockdep warning:

 ============================================
 WARNING: possible recursive locking detected
 6.2.0-rc5-00205-gc96618275234 #56 Not tainted
 --------------------------------------------
 repro/8607 is trying to acquire lock:
 ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: l2tp_tunnel_register+0x2b7/0x11c0

 but task is already holding lock:
 ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

 other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(sk_lock-AF_PPPOX);
   lock(sk_lock-AF_PPPOX);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

 1 lock held by repro/8607:
  #0: ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

 stack backtrace:
 CPU: 0 PID: 8607 Comm: repro Not tainted 6.2.0-rc5-00205-gc96618275234 #56
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
 Call Trace:
  <TASK>
  dump_stack_lvl+0x100/0x178
  __lock_acquire.cold+0x119/0x3b9
  ? lockdep_hardirqs_on_prepare+0x410/0x410
  lock_acquire+0x1e0/0x610
  ? l2tp_tunnel_register+0x2b7/0x11c0
  ? lock_downgrade+0x710/0x710
  ? __fget_files+0x283/0x3e0
  lock_sock_nested+0x3a/0xf0
  ? l2tp_tunnel_register+0x2b7/0x11c0
  l2tp_tunnel_register+0x2b7/0x11c0
  ? sprintf+0xc4/0x100
  ? l2tp_tunnel_del_work+0x6b0/0x6b0
  ? debug_object_deactivate+0x320/0x320
  ? lockdep_init_map_type+0x16d/0x7a0
  ? lockdep_init_map_type+0x16d/0x7a0
  ? l2tp_tunnel_create+0x2bf/0x4b0
  ? l2tp_tunnel_create+0x3c6/0x4b0
  pppol2tp_connect+0x14e1/0x1a30
  ? pppol2tp_put_sk+0xd0/0xd0
  ? aa_sk_perm+0x2b7/0xa80
  ? aa_af_perm+0x260/0x260
  ? bpf_lsm_socket_connect+0x9/0x10
  ? pppol2tp_put_sk+0xd0/0xd0
  __sys_connect_file+0x14f/0x190
  __sys_connect+0x133/0x160
  ? __sys_connect_file+0x190/0x190
  ? lockdep_hardirqs_on+0x7d/0x100
  ? ktime_get_coarse_real_ts64+0x1b7/0x200
  ? ktime_get_coarse_real_ts64+0x147/0x200
  ? __audit_syscall_entry+0x396/0x500
  __x64_sys_connect+0x72/0xb0
  do_syscall_64+0x38/0xb0
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

This patch fixes the issue by getting/creating the tunnel before
locking the pppol2tp socket.

Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
v2: Get/create the tunnel before locking the pppol2tp socket suggested
    by Guillaume Nault.
v1: https://lore.kernel.org/all/20230130154438.1373750-1-syoshida@redhat.com/

 net/l2tp/l2tp_ppp.c | 118 ++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 54 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index db2e584c625e..68d02e217ca3 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -650,6 +650,65 @@ static int pppol2tp_tunnel_mtu(const struct l2tp_tunnel *tunnel)
 	return mtu - PPPOL2TP_HEADER_OVERHEAD;
 }
 
+static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
+					       struct l2tp_connect_info *info,
+					       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;
 
-	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.
-	 */
-	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) {
-				error = -EBADF;
-				goto end;
-			}
-
-			error = l2tp_tunnel_create(info.fd,
-						   info.version,
-						   info.tunnel_id,
-						   info.peer_tunnel_id, &tcfg,
-						   &tunnel);
-			if (error < 0)
-				goto end;
-
-			l2tp_tunnel_inc_refcount(tunnel);
-			error = l2tp_tunnel_register(tunnel, sock_net(sk),
-						     &tcfg);
-			if (error < 0) {
-				kfree(tunnel);
-				goto end;
-			}
-			drop_tunnel = true;
-			new_tunnel = true;
-		}
-	} else {
-		/* Error if we can't find the tunnel */
-		error = -ENOENT;
-		if (!tunnel)
-			goto end;
-
-		/* Error if socket is not prepped */
-		if (!tunnel->sock)
-			goto end;
-	}
-
 	if (tunnel->peer_tunnel_id == 0)
 		tunnel->peer_tunnel_id = info.peer_tunnel_id;
 
@@ -840,8 +851,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 	}
 	if (drop_refcnt)
 		l2tp_session_dec_refcount(session);
-	if (drop_tunnel)
-		l2tp_tunnel_dec_refcount(tunnel);
+	l2tp_tunnel_dec_refcount(tunnel);
 	release_sock(sk);
 
 	return error;
-- 
2.39.0

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Guillaume Nault]

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.

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Alexander Lobakin]

From: Shigeru Yoshida <syoshida@redhat.com>
Date: Mon, 13 Feb 2023 01:26:23 +0900

> When a file descriptor of pppol2tp socket is passed as file descriptor
> of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
> This situation is reproduced by the following program:

[...]

> +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
> +					       struct l2tp_connect_info *info,
> +					       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) {

This `if` is the sole thing the outer `if` contains, could we combine them?

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

This fits into the prev line.

> +						   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);

[...]

Thanks,
Olek

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Guillaume Nault]

On Mon, Feb 13, 2023 at 04:05:59PM +0100, Alexander Lobakin wrote:
> From: Shigeru Yoshida <syoshida@redhat.com>
> Date: Mon, 13 Feb 2023 01:26:23 +0900
> 
> > When a file descriptor of pppol2tp socket is passed as file descriptor
> > of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
> > This situation is reproduced by the following program:
> 
> [...]
> 
> > +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
> > +					       struct l2tp_connect_info *info,
> > +					       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) {
> 
> This `if` is the sole thing the outer `if` contains, could we combine them?

The logic of this code is a bit convoluted, sure, but if we want to
rework it, let's simplify it for real:

	tunnel = l2tp_tunnel_get(...)
	if (tunnel)
		return tunnel; /* the original !tunnel->sock test is useless I believe */

	/* Tunnel not found. Try to create one if both session_id and
	 * peer_session_id are 0. Fail otherwise.
	 */
	if (info->session_id || info->peer_session_id)
		return ERR_PTR(-ENOENT);

	[...] /* Tunnel creation code */


However, I'd prefer to keep such refactoring for later. Keeping the
same structure in pppol2tp_tunnel_get() as in the original code helps
reviewing the patch.

> > +			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,
> 
> This fits into the prev line.
> 
> > +						   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);
> 
> [...]
> 
> Thanks,
> Olek
> 

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Shigeru Yoshida]

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

> 

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Shigeru Yoshida]

Hi Olek,

On Mon, Feb 13, 2023 at 04:05:59PM +0100, Alexander Lobakin wrote:
> From: Shigeru Yoshida <syoshida@redhat.com>
> Date: Mon, 13 Feb 2023 01:26:23 +0900
> 
> > When a file descriptor of pppol2tp socket is passed as file descriptor
> > of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
> > This situation is reproduced by the following program:
> 
> [...]
> 
> > +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
> > +					       struct l2tp_connect_info *info,
> > +					       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) {
> 
> This `if` is the sole thing the outer `if` contains, could we combine them?

Thank you so much for your comment.  Yes, I agree with you.  But I'd
like to keep the original structure as Guillaume suggested.

Thanks,
Shigeru

> 
> > +			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,
> 
> This fits into the prev line.
> 
> > +						   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);
> 
> [...]
> 
> Thanks,
> Olek
> 

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Guillaume Nault]

On Wed, Feb 15, 2023 at 01:49:08AM +0900, Shigeru Yoshida wrote:
> 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?

Yes. Networking fixes should be based on the "net" tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

For more details about posting patches to netdev, you can check
Documentation/process/maintainer-netdev.rst

Or the online version:
https://kernel.org/doc/html/latest/process/maintainer-netdev.html

> Thanks,
> Shigeru
> 
> > 
> 

Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()

[Shigeru Yoshida]

Hi Guillaume,

On Tue, Feb 14, 2023 at 06:09:33PM +0100, Guillaume Nault wrote:
> On Wed, Feb 15, 2023 at 01:49:08AM +0900, Shigeru Yoshida wrote:
> > 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?
> 
> Yes. Networking fixes should be based on the "net" tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

Thanks.  I'll use the "net" tree for v3 patch.

> For more details about posting patches to netdev, you can check
> Documentation/process/maintainer-netdev.rst
> 
> Or the online version:
> https://kernel.org/doc/html/latest/process/maintainer-netdev.html

Got it.  I'll check that.

Anyway, I'll make v3 patch based on your feedback.

Thanks,
Shigeru

> 
> > Thanks,
> > Shigeru
> > 
> > > 
> > 
>