netdev - [BUG/Q] can_pernet_exit() leaves devices on dead net

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: <8c941b2d-0c90-ba6c-6020-23530b146fd9@virtuozzo.com>
Date:   Thu, 1 Mar 2018 18:53:51 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     mkl@...gutronix.de, socketcan@...tkopp.net, davem@...emloft.net,
        linux-can@...r.kernel.org, netdev@...r.kernel.org, dev@...herer.org
Subject: [BUG/Q] can_pernet_exit() leaves devices on dead net

Hi,

I'm converting/reviewing pernet_operations either they allow several net namespaces
to be created/destroyed in parallel or not. Please, see the details in my recent
patches in net-next.git, if your are interested.

There is a strange place in can_pernet_ops pernet subsys, I found:

static void can_pernet_exit(struct net *net)
{
	...
	rcu_read_lock();
	for_each_netdev_rcu(net, dev) {
		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
			struct can_dev_rcv_lists *d = dev->ml_priv; 

			BUG_ON(d->entries);
			kfree(d);
			dev->ml_priv = NULL;
		}
	}
	rcu_read_unlock()
	...
}

This code clears dev->ml_priv from can devices, and it looks strange.
Since can_pernet_ops is pernet subsys, it's executed after default_device_exit()
from default_device_ops pernet device, as devices exit methods are executed first
(see net/core/net_namespace.c).

There are no NETIF_F_NETNS_LOCAL devices among can devices, though there is
check of can_link_ops in safe_candev_priv(). I haven't found can devices may
have net_device::rtnl_link_ops. But the code seems want to allow them. Anyway,
it's wrong in any case:

1)If there are can devices, which may be skipped by default_device_exit(),
can_pernet_exit() must use rtnl_lock() instead of rcu_read_lock(), and
it must move such devices to init_net. See wifi cfg80211_pernet_exit() for example.

2)If there are no such the devices, the code between rcu_read_lock() and rcu_read_unlock()
is useless, and must be deleted, as it never works and confuses a reader.

Thanks,
Kirill