From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-db5eur01on0104.outbound.protection.outlook.com ([104.47.2.104]:48114 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752206AbeCPJTg (ORCPT ); Fri, 16 Mar 2018 05:19:36 -0400 Subject: Re: [BUG/Q] can_pernet_exit() leaves devices on dead net To: Oliver Hartkopp , mkl@pengutronix.de, linux-can@vger.kernel.org, netdev@vger.kernel.org, dev@kicherer.org References: <8c941b2d-0c90-ba6c-6020-23530b146fd9@virtuozzo.com> <38145688-1c15-3df4-c127-591959a759ff@hartkopp.net> <21bafe4a-984f-2897-b092-270ccda136f0@virtuozzo.com> <9f3585bb-4cf9-b0e1-3069-37fbae2ce54b@hartkopp.net> From: Kirill Tkhai Message-ID: Date: Fri, 16 Mar 2018 12:19:29 +0300 MIME-Version: 1.0 In-Reply-To: <9f3585bb-4cf9-b0e1-3069-37fbae2ce54b@hartkopp.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: 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