Messages in this thread | | | Date | Wed, 15 Feb 2023 01:49:08 +0900 | From | Shigeru Yoshida <> | Subject | Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register() |
| |
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
>
| |