[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y+u8XKx0k9ow8/NA@kernel-devel>
Date: Wed, 15 Feb 2023 01:52:44 +0900
From: Shigeru Yoshida <syoshida@...hat.com>
To: Alexander Lobakin <alexandr.lobakin@...el.com>
Cc: jchapman@...alix.com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, gnault@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in
l2tp_tunnel_register()
Hi Olek,
On Mon, Feb 13, 2023 at 04:05:59PM +0100, Alexander Lobakin wrote:
> From: Shigeru Yoshida <syoshida@...hat.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
>
Powered by blists - more mailing lists