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

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 06.03.2018 13:26, Oliver Hartkopp wrote:
> - DaveM
> 
> Hi Kirill,
> 
> On 03/05/2018 04:22 PM, Kirill Tkhai wrote:
> 
>> 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.
> 
> In fact most of the real CAN drivers have rtnl_link_ops:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L1162
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L1225
> 
> Just slcan.c which is something like slip.c is missing these feature.
> 
> AFAIK real CAN netdevices are created in init_net at system startup, and you
> can move them to a netns later.
> 
> When we already have rtnl_link_ops in the real CAN drivers - what happens to
> them when the namespace is destroyed? Are they still moved to init_net, or do
> we need to add some default handler in the current rtnl_link_ops setup?

They are unregistered in default_device_exit_batch():

        list_for_each_entry(net, net_list, exit_list) {
                for_each_netdev_reverse(net, dev) {
                        if (dev->rtnl_link_ops && dev->rtnl_link_ops->dellink)
                                dev->rtnl_link_ops->dellink(dev, &dev_kill_list);
                        else
                                unregister_netdevice_queue(dev, &dev_kill_list);
                }
        }
        unregister_netdevice_many(&dev_kill_list);

So that, my question in the start of the topic is about that. If we unregister them,
what devices we're going to meet in can_pernet_exit()?

Also, can devices not having rtnl_link_ops are moved into init_net in default_device_exit():

for_each_netdev_safe(net, dev, aux) {
                if (dev->features & NETIF_F_NETNS_LOCAL)
                        continue;

                /* Leave virtual devices for the generic cleanup */
                if (dev->rtnl_link_ops)
                        continue;

                err = dev_change_net_namespace(dev, &init_net, fb_name);
}
 
>> 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.
> 
> See above?
> 
>>>> 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 was a patchset of 14 patches from Marc where some of the refactoring &
> renaming already made it into mainline - but the patches to move the
> can_dev_rcv_lists data structure into the network device space have not been
> pushed:
> 
> https://marc.info/?l=linux-can&m=150169588319315&w=2
> https://marc.info/?l=linux-can&r=1&b=201708&w=2
> 
> This patch & documentation describes Marc's proposed idea best:
> https://marc.info/?l=linux-can&m=150169589619340&w=2

Yes, this one looks good:
https://marc.info/?l=linux-can&m=150169589119335&w=2

Regards,
Kirill