netdev - Re: [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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38145688-1c15-3df4-c127-591959a759ff@hartkopp.net>
Date:   Mon, 5 Mar 2018 14:59:52 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>, mkl@...gutronix.de,
        davem@...emloft.net, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, dev@...herer.org
Subject: Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

Hi Kirill,

On 03/01/2018 04:53 PM, Kirill Tkhai wrote:

> 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.

Thanks for your effort to review all these different sites!

> 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.

To give some more background about these 'struct can_dev_rcv_lists':

The receive lists are managed by the AF_CAN framework in linux/net/can for
each CAN network device. When the per-net modules like can-raw, can-bcm or
can-gw are removed (or if there are no more open sockets or the netdevices are
removed) the CAN filters are removed too.

Finally - when can.ko is removed - the filters should be cleared (that's why
the BUG() statement checks the emptiness) and then the empty can_dev_rcv_lists
structure is free'd.

> 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).

Hm - a device exit fires the NETDEV_UNREGISTER notifier which removes the
user-generated filters (e.g. in raw_notifier() in net/can/raw.c).
Finally the can_dev_rcv_lists structure is free'd in af_can.c.

Marc Kleine-Budde recently proposed a patch to create the can_dev_rcv_lists at
netdevice creation time (-> the space is allocated by alloc_netdev() and
removed by free_netdev()). This would remove the handling (allocate & free) of
ml_priv by af_can.c. Would this rework fix the described issue?

> 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.

We use rtnl_link_ops to create and remove virtual CAN interfaces (vcan.c and
vxcan.c) and to alter MTU values and bitrates for real CAN interfaces.

See:

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/can.txt#L1001

https://elixir.bootlin.com/linux/latest/source/Documentation/networking/can.txt#L1041

> 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.

The latter would create a memory leak. Maybe the suggested change from Marc
would solve the entire problem then?

Thanks & best regards,
Oliver