netdev - Refcount mismatch when unregistering netdevice from kernel

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-next>] [day] [month] [year] [list]

Message-ID: <ca64de092db5a2ac80d22eaa9d662520@codeaurora.org>
Date:   Mon, 07 Dec 2020 20:55:51 -0700
From:   stranche@...eaurora.org
To:     dsahern@...il.com, weiwan@...gle.com, kafai@...com,
        maheshb@...gle.com, kuba@...nel.org, netdev@...r.kernel.org
Cc:     subashab@...eaurora.org
Subject: Refcount mismatch when unregistering netdevice from kernel

Hi everyone,

We've recently been investigating a refcount problem when unregistering 
a netdevice from the kernel. It seems that the IPv6 module is still 
holding various references to the inet6_dev associated with the main 
netdevice struct that are not being released, preventing the 
unregistration from completing.

After tracing the various locations that take/release refcounts to these 
two structs, we see that there are mismatches in the refcount for the 
inet6_dev in the DST path when there are routes flushed with the 
rt6_uncached_list_flush_dev() function during rt6_disable_ip() when the 
device is unregistering. It seems that usually these references are 
freed via ip6_dst_ifdown() in the dst_ops->ifdown callback from 
dst_dev_put(), but this callback is not executed in the 
rt6_uncached_list_flush_dev() function. Instead, a reference to the 
inet6_dev is simply dropped to account for the inet6_dev_get() in 
ip6_rt_copy_init().

Unfortunately, this does not seem to be sufficient, as these uncached 
routes have an additional refcount on the inet6_device attached to them 
from the DST allocation. In the normal case, this reference from the DST 
allocation will happen in the dst_ops->destroy() callback in the 
dst_destroy() function when the DST is being freed. However, since 
rt6_uncached_list_flush_dev() changes the inet6_device stored in the DST 
to the loopback device, the dst_ops->destroy() callback doesn't 
decrement the refcount on the correct inet6_dev struct.

We're wondering if it would be appropriate to put() the refcount 
additionally for the uncached routes when flushing out the list for the 
unregistering device. Perhaps something like the following?

diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 6602f43..554b07b 
100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -156,9 +156,11 @@ void rt6_uncached_list_del(struct rt6_info *rt)

   char rt6_uncached_list_flush_dev_log1[1000][512];
   int rt6_uncached_list_flush_dev_log1_iter = 0; -static void 
rt6_uncached_list_flush_dev(struct net *net, struct net_device *dev)
+static void rt6_uncached_list_flush_dev(struct net *net, struct 
net_device *dev,
+                                        unsigned long event)
   {
          struct net_device *loopback_dev = net->loopback_dev;
+       bool unreg = event == NETDEV_UNREGISTER;
          int cpu;

          if (dev == loopback_dev)
@@ -190,6 +192,10 @@ static void rt6_uncached_list_flush_dev(struct net 
*net, struct net_device *dev)
                          }

                          if (rt_idev->dev == dev) {
+                               if (rt->dst.ops->ifdown)
+                                       rt->dst.ops->ifdown(&rt->dst, 
dev,
+                                                           unreg);
+
                                  rt->rt6i_idev = 
in6_dev_get(loopback_dev);
                                  in6_dev_put(rt_idev);
                          }
@@ -4781,7 +4787,7 @@ void rt6_sync_down_dev(struct net_device *dev, 
unsigned long event)
   void rt6_disable_ip(struct net_device *dev, unsigned long event)
   {
          rt6_sync_down_dev(dev, event);
-       rt6_uncached_list_flush_dev(dev_net(dev), dev);
+       rt6_uncached_list_flush_dev(dev_net(dev), dev, event);
          neigh_ifdown(&nd_tbl, dev);
   }

For reference, here are some samples of the refcounts on the 
inet6_device. In this log we saw the inet6_device had a final refcount 
of 4 while unregistering.
holds                 puts
ip6_rt_copy_init 17   13 ip6_dst_ifdown
xfrm6_fill_dst   6     5 xfrm6_dst_destroy
                        1 rt6_disable_ip
icmp6_dst_alloc  28   25 ip6_dst_destroy
                        3 rt6_disable_ip

Thanks,
Sean