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.
> 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?
> 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?
Thanks & best regards,
Oliver
