netdev - Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170812180129.GA31700@splinter>
Date:   Sat, 12 Aug 2017 21:01:29 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Wei Wang <weiwan@...gle.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Martin KaFai Lau <kafai@...com>,
        lkml <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>
Subject: Re: unregister_netdevice: waiting for eth0 to become free. Usage
 count = 1

Hi Wei,

On Fri, Aug 11, 2017 at 05:10:02PM -0700, Wei Wang wrote:
> I think we have a potential fix for this issue.
> Martin and I found that when addrconf_dst_alloc() creates a rt6, it is
> possible that rt6->dst.dev points to loopback device while
> rt6->rt6i_idev->dev points to a real device.
> When the real device goes down, the current fib6 clean up code only
> checks for rt6->dst.dev and assumes rt6->rt6i_idev->dev is the same.
> That leaves unreleased refcnt on the real device if rt6->dst.dev
> points to loopback dev.

[...]

> From 2d8861808c2029013f6b6e86120ba6902329145b Mon Sep 17 00:00:00 2001
> From: Wei Wang <weiwan@...gle.com>
> Date: Fri, 11 Aug 2017 16:36:04 -0700
> Subject: [PATCH 1/2] potential fix for unregister_netdevice()
> 
> Change-Id: I5d5f6f7a7ad0f5dd769f33487db17ff2570d52ea
> ---
>  net/ipv6/route.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96a819d..105922903932 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -417,14 +417,12 @@ static void ip6_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
>  	struct net_device *loopback_dev =
>  		dev_net(dev)->loopback_dev;
>  
> -	if (dev != loopback_dev) {
> -		if (idev && idev->dev == dev) {
> -			struct inet6_dev *loopback_idev =
> -				in6_dev_get(loopback_dev);
> -			if (loopback_idev) {
> -				rt->rt6i_idev = loopback_idev;
> -				in6_dev_put(idev);
> -			}
> +	if (idev && idev->dev != loopback_dev) {
> +		struct inet6_dev *loopback_idev =
> +			in6_dev_get(loopback_dev);
> +		if (loopback_idev) {
> +			rt->rt6i_idev = loopback_idev;
> +			in6_dev_put(idev);
>  		}
>  	}
>  }
> @@ -2789,7 +2787,8 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
>  	const struct arg_dev_net *adn = arg;
>  	const struct net_device *dev = adn->dev;
>  
> -	if ((rt->dst.dev == dev || !dev) &&
> +	if ((rt->dst.dev == dev || !dev ||
> +	     rt->rt6i_idev->dev == dev) &&

Can you please explain why this line is needed? While host routes aren't
removed from the FIB by rt6_ifdown() (when dst.dev goes down), they are
removed later on in addrconf_ifdown().

With your patch, if I check the return value of ip6_del_rt() in
__ipv6_ifa_notify() I see that -ENONET is returned. Because the host
route was already removed by rt6_ifdown(). When the line in question is
removed from the patch I don't get the error anymore.

Is it possible that in John's case the host route was correctly removed
from the FIB and that the unreleased reference was due to a wrong check
in ip6_dst_ifdown() (which you patched correctly AFAICT)?

Thanks

>  	    rt != adn->net->ipv6.ip6_null_entry &&
>  	    (rt->rt6i_nsiblings == 0 ||
>  	     (dev && netdev_unregistering(dev)) ||
> -- 
> 2.14.0.434.g98096fd7a8-goog
>