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