Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()
From: Shigeru Yoshida
Date: Tue Feb 14 2023 - 11:51:01 EST
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
>