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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Mar 2018 11:26:44 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>, mkl@...gutronix.de,
        linux-can@...r.kernel.org, netdev@...r.kernel.org, dev@...herer.org
Subject: Re: [BUG/Q] can_pernet_exit() leaves devices on dead net

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

> 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

Best regards,
Oliver