Messages in this thread | | | Date | Thu, 2 Feb 2023 00:43:49 +0900 | From | Shigeru Yoshida <> | Subject | Re: [PATCH] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register() |
| |
Hi Guillaume,
On Mon, Jan 30, 2023 at 06:03:52PM +0100, Guillaume Nault wrote: > On Tue, Jan 31, 2023 at 12:44:38AM +0900, Shigeru Yoshida wrote: > > This patch fixes the issue by returning error when a pppol2tp socket > > itself is passed. > > Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()") > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > > --- > > net/l2tp/l2tp_ppp.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > > index db2e584c625e..88d1a339500b 100644 > > --- a/net/l2tp/l2tp_ppp.c > > +++ b/net/l2tp/l2tp_ppp.c > > @@ -702,11 +702,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr, > > struct l2tp_tunnel_cfg tcfg = { > > .encap = L2TP_ENCAPTYPE_UDP, > > }; > > + int dummy = 0; > > There's no need to initialise dummy here. This is just confusing. > We could even do without any extra variable and reuse error in > sockfd_lookup(). > > > /* Prevent l2tp_tunnel_register() from trying to set up > > - * a kernel socket. > > + * a kernel socket. Also, prevent l2tp_tunnel_register() > > + * from trying to use pppol2tp socket itself. > > */ > > - if (info.fd < 0) { > > + if (info.fd < 0 || > > + sock == sockfd_lookup(info.fd, &dummy)) { > > error = -EBADF; > > goto end; > > } > > That should work, but the real problem is calling l2tp_tunnel_register() > under lock_sock(). We should instead get/create the tunnel before > locking the pppol2tp socket.
Thank you so much for your comment, and sorry for the late response.
Do you mean we can call l2tp_tunnel_register() without pppol2tp socket lock? I've read the source code of pppol2tp_connect(), but I'm not sure why pppol2tp socket is locked at the beginning of this function.
If we can call l2tp_tunnel_register() without pppol2tp socket lock, I think we can move lock_sock() after l2tp_tunnel_register().
Thanks, Shigeru
>
| |