diff mbox series

[5.10,09/38] llc: fix netdevice reference leaks in llc_ui_bind()

Message ID 20220325150420.029041400@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg KH March 25, 2022, 3:04 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

commit 764f4eb6846f5475f1244767d24d25dd86528a4a upstream.

Whenever llc_ui_bind() and/or llc_ui_autobind()
took a reference on a netdevice but subsequently fail,
they must properly release their reference
or risk the infamous message from unregister_netdevice()
at device dismantle.

unregister_netdevice: waiting for eth0 to become free. Usage count = 3

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: 赵子轩 <beraphin@gmail.com>
Reported-by: Stoyan Manolov <smanolov@suse.de>
Link: https://lore.kernel.org/r/20220323004147.1990845-1-eric.dumazet@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/llc/af_llc.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Pavel Machek March 26, 2022, 8:09 p.m. UTC | #1 
Hi!

> From: Eric Dumazet <edumazet@google.com>
> 
> commit 764f4eb6846f5475f1244767d24d25dd86528a4a upstream.
> 
> Whenever llc_ui_bind() and/or llc_ui_autobind()
> took a reference on a netdevice but subsequently fail,
> they must properly release their reference
> or risk the infamous message from unregister_netdevice()
> at device dismantle.
> 
> unregister_netdevice: waiting for eth0 to become free. Usage count =
> 3

Can someone check this? AFAICT this is buggy.

static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
{
        struct sock *sk = sock->sk;
        struct llc_sock *llc = llc_sk(sk);
        struct llc_sap *sap;
        int rc = -EINVAL;

        if (!sock_flag(sk, SOCK_ZAPPED))
                goto out;

There are 'goto out's from both before dev_get() and after it,
dev_put() will be called with NULL pointer. dev_put() can't handle
NULL at least in the old kernels... this is simply confused.

Mainline has dev_put_track() there, but I see same confusion.

Best regards,
								Pavel


> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -311,6 +311,10 @@ static int llc_ui_autobind(struct socket
>  	sock_reset_flag(sk, SOCK_ZAPPED);
>  	rc = 0;
>  out:
> +	if (rc) {
> +		dev_put(llc->dev);
> +		llc->dev = NULL;
> +	}
>  	return rc;
>  }
>  
> @@ -409,6 +413,10 @@ static int llc_ui_bind(struct socket *so
>  out_put:
>  	llc_sap_put(sap);
>  out:
> +	if (rc) {
> +		dev_put(llc->dev);
> +		llc->dev = NULL;
> +	}
>  	release_sock(sk);
>  	return rc;
>  }
>
Jakub Kicinski March 26, 2022, 8:13 p.m. UTC | #2 
On Sat, 26 Mar 2022 21:09:22 +0100 Pavel Machek wrote:
> Can someone check this? AFAICT this is buggy.
> 
> static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> {
>         struct sock *sk = sock->sk;
>         struct llc_sock *llc = llc_sk(sk);
>         struct llc_sap *sap;
>         int rc = -EINVAL;
> 
>         if (!sock_flag(sk, SOCK_ZAPPED))
>                 goto out;
> 
> There are 'goto out's from both before dev_get() and after it,
> dev_put() will be called with NULL pointer. dev_put() can't handle
> NULL at least in the old kernels... this is simply confused.
> 
> Mainline has dev_put_track() there, but I see same confusion.
> 
> Best regards,

commit 2d327a79ee17 ("llc: only change llc->dev when bind() succeeds"),
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=2d327a79ee176930dc72c131a970c891d367c1dc

Should be in mainline on Thursday, LMK if we need to accelerate.
IDK if anyone enables LLC2.
Pavel Machek March 26, 2022, 9:35 p.m. UTC | #3 
Hi!

> > Can someone check this? AFAICT this is buggy.
> > 
> > static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> > {
> >         struct sock *sk = sock->sk;
> >         struct llc_sock *llc = llc_sk(sk);
> >         struct llc_sap *sap;
> >         int rc = -EINVAL;
> > 
> >         if (!sock_flag(sk, SOCK_ZAPPED))
> >                 goto out;
> > 
> > There are 'goto out's from both before dev_get() and after it,
> > dev_put() will be called with NULL pointer. dev_put() can't handle
> > NULL at least in the old kernels... this is simply confused.
> > 
> > Mainline has dev_put_track() there, but I see same confusion.
> > 
> > Best regards,
> 
> commit 2d327a79ee17 ("llc: only change llc->dev when bind() succeeds"),
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=2d327a79ee176930dc72c131a970c891d367c1dc
> 
> Should be in mainline on Thursday, LMK if we need to accelerate.
> IDK if anyone enables LLC2.

Thank you, yes, that looks good at the fast glance.

But this patch does more harm than good on its own, so I believe it
should be dropped for now, and only queued when the fixes are
available.

Best regards,
								Pavel
Greg KH March 27, 2022, 9:51 a.m. UTC | #4 
On Sat, Mar 26, 2022 at 01:13:25PM -0700, Jakub Kicinski wrote:
> On Sat, 26 Mar 2022 21:09:22 +0100 Pavel Machek wrote:
> > Can someone check this? AFAICT this is buggy.
> > 
> > static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> > {
> >         struct sock *sk = sock->sk;
> >         struct llc_sock *llc = llc_sk(sk);
> >         struct llc_sap *sap;
> >         int rc = -EINVAL;
> > 
> >         if (!sock_flag(sk, SOCK_ZAPPED))
> >                 goto out;
> > 
> > There are 'goto out's from both before dev_get() and after it,
> > dev_put() will be called with NULL pointer. dev_put() can't handle
> > NULL at least in the old kernels... this is simply confused.
> > 
> > Mainline has dev_put_track() there, but I see same confusion.
> > 
> > Best regards,
> 
> commit 2d327a79ee17 ("llc: only change llc->dev when bind() succeeds"),
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=2d327a79ee176930dc72c131a970c891d367c1dc
> 
> Should be in mainline on Thursday, LMK if we need to accelerate.
> IDK if anyone enables LLC2.

I'll queue this up now, thanks.

greg k-h
Pavel Machek March 28, 2022, 9:08 a.m. UTC | #5 
Hi!

> > > Can someone check this? AFAICT this is buggy.
> > > 
> > > static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> > > {
> > >         struct sock *sk = sock->sk;
> > >         struct llc_sock *llc = llc_sk(sk);
> > >         struct llc_sap *sap;
> > >         int rc = -EINVAL;
> > > 
> > >         if (!sock_flag(sk, SOCK_ZAPPED))
> > >                 goto out;
> > > 
> > > There are 'goto out's from both before dev_get() and after it,
> > > dev_put() will be called with NULL pointer. dev_put() can't handle
> > > NULL at least in the old kernels... this is simply confused.
> > > 
> > > Mainline has dev_put_track() there, but I see same confusion.
> > > 
> > > Best regards,
> > 
> > commit 2d327a79ee17 ("llc: only change llc->dev when bind() succeeds"),
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=2d327a79ee176930dc72c131a970c891d367c1dc
> > 
> > Should be in mainline on Thursday, LMK if we need to accelerate.
> > IDK if anyone enables LLC2.
> 
> I'll queue this up now, thanks.

As the changelog says, this needs b37a46683739, otherwise there will
be oops-es in even more cases.

Best regards,
								Pavel
Pavel Machek March 28, 2022, 9:37 a.m. UTC | #6 
Hi!

> > > > commit 2d327a79ee17 ("llc: only change llc->dev when bind() succeeds"),
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=2d327a79ee176930dc72c131a970c891d367c1dc
> > > > 
> > > > Should be in mainline on Thursday, LMK if we need to accelerate.
> > > > IDK if anyone enables LLC2.
> > > 
> > > I'll queue this up now, thanks.
> > 
> > As the changelog says, this needs b37a46683739, otherwise there will
> > be oops-es in even more cases.
> 
> If you look at the change, I think I already handled that issue.  If
> not, please let me know.

Actually, AFAICT it will now oops even in the common (non-error) path
in llc_ui_autobind().

Best regards,
								Pavel
Greg KH March 28, 2022, 9:42 a.m. UTC | #7 
On Mon, Mar 28, 2022 at 11:31:16AM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > Should be in mainline on Thursday, LMK if we need to accelerate.
> > > > > IDK if anyone enables LLC2.
> > > > 
> > > > I'll queue this up now, thanks.
> > > 
> > > As the changelog says, this needs b37a46683739, otherwise there will
> > > be oops-es in even more cases.
> > 
> > If you look at the change, I think I already handled that issue.  If
> > not, please let me know.
> 
> I did not notice you making changes there, but no, it is not correct
> AFAICT.
> 
> # commit 163960a7de1333514c9352deb7c80c6b9fd9abf2
> # Author: Eric Dumazet <edumazet@google.com>
> # Date:   Thu Mar 24 20:58:27 2022 -0700
> 
> #    llc: only change llc->dev when bind() succeeds
> ...    
> #     Make sure commit b37a46683739 ("netdevice: add the case if dev is NULL")
> #     is already present in your trees.
> 
> Before b37a46683739, dev_put can't handle NULL.
>     
> +++ b/net/llc/af_llc.c
> @@ -287,14 +288,14 @@ static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
> ...
> 
> -		llc->dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd);
> -	if (!llc->dev)
> +		dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd);
> +	if (!dev)
>  		goto out;
>  	rc = -EUSERS;
>  	llc->laddr.lsap = llc_ui_autoport();
> 
> One of several paths where we goto out with dev==NULL.
> 
> @@ -311,10 +317,7 @@ static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr)
>  	sock_reset_flag(sk, SOCK_ZAPPED);
>  	rc = 0;
>  out:
> -	if (rc) {
> -		dev_put(llc->dev);
> -		llc->dev = NULL;
> -	}
> +	dev_put(dev);
>  	return rc;
>  }
> 
> 
> But dev_put can't handle NULL.

Ah, missed that one.  I'll go queue up b37a46683739 now.

thanks,

greg k-h
diff mbox series

Patch

--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -311,6 +311,10 @@  static int llc_ui_autobind(struct socket
 	sock_reset_flag(sk, SOCK_ZAPPED);
 	rc = 0;
 out:
+	if (rc) {
+		dev_put(llc->dev);
+		llc->dev = NULL;
+	}
 	return rc;
 }
 
@@ -409,6 +413,10 @@  static int llc_ui_bind(struct socket *so
 out_put:
 	llc_sap_put(sap);
 out:
+	if (rc) {
+		dev_put(llc->dev);
+		llc->dev = NULL;
+	}
 	release_sock(sk);
 	return rc;
 }