Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

[Kirill Tkhai <ktkhai@virtuozzo.com>]

Hi, Oliver,

thanks for your reply.

On 05.03.2018 16:59, Oliver Hartkopp wrote:
> 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.

Thanks for the explanation, and module unloading should be nice. Just to clarify,
I worry not about rules, but about netdevices.

	unshare -n ip link add type vcan

This command creates net ns, adds vcan there and exits. Then net ns is destroyed.
Since vcan has rtnl_link_ops, it unregistered by default_device_exit_batch().
Real can devices are moved to init_net in default_device_exit(), as they don't
have rtnl_link_ops set.

So, for_each_netdev_rcu() cycle in can_pernet_exit() should be useless (there are
no can devices in the list of net's devices). This looks so for me, please say
what devices are there if my assumption is wrong.

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

Could you please give me a link to the patches? I can't find them in patchwork.

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

Kirill