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
