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

On 06.03.2018 13:26, Oliver Hartkopp wrote:

- DaveM

Hi Kirill,

On 03/05/2018 04:22 PM, Kirill Tkhai wrote:

quoted

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);
}
 

quoted

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?

quoted

quoted

quoted

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