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

From: Shigeru Yoshida
Date: Tue Feb 14 2023 - 11:53:45 EST


Hi Olek,

On Mon, Feb 13, 2023 at 04:05:59PM +0100, Alexander Lobakin wrote:
> From: Shigeru Yoshida <syoshida@xxxxxxxxxx>
> 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
>