Hi,

On 20/09/2024 11:32, Kuniyuki Iwashima wrote:

From: Antonio Quartulli <antonio@openvpn.net>
Date: Thu, 19 Sep 2024 13:57:51 +0200

quoted

Hi Kuniyuki and thank you for chiming in.

On 19/09/2024 07:52, Kuniyuki Iwashima wrote:

quoted

From: Antonio Quartulli <antonio@openvpn.net>
Date: Tue, 17 Sep 2024 03:07:12 +0200

quoted

+/* we register with rtnl to let core know that ovpn is a virtual driver and
+ * therefore ifaces should be destroyed when exiting a netns
+ */
+static struct rtnl_link_ops ovpn_link_ops = {
+};

This looks like abusing rtnl_link_ops.

In some way, the inspiration came from
5b9e7e160795 ("openvswitch: introduce rtnl ops stub")

[which just reminded me that I wanted to fill the .kind field, but I
forgot to do so]

The reason for taking this approach was to avoid handling the iface
destruction upon netns exit inside the driver, when the core already has
all the code for taking care of this for us.

Originally I implemented pernet_operations.pre_exit, but Sabrina
suggested that letting the core handle the destruction was cleaner (and
I agreed).

However, after I removed the pre_exit implementation, we realized that
default_device_exit_batch/default_device_exit_net thought that an ovpn
device is a real NIC and was moving it to the global netns rather than
killing it.

One way to fix the above was to register rtnl_link_ops with netns_fund =
false (so the ops object you see in this patch is not truly "empty").

However, I then hit the bug which required patch 2 to get fixed.

Does it make sense to you?
Or you still think this is an rtnl_link_ops abuse?

The use of .kind makes sense, and the change should be in this patch.

Ok, will add it here and I will also add an explicit .netns_fund = false 
to highlight the fact that we need this attribute to avoid moving the 
iface to the global netns.

For the patch 2 and dellink(), is the device not expected to be removed
by ip link del ?  Setting unregister_netdevice_queue() to dellink() will
support RTM_DELLINK, but otherwise -EOPNOTSUPP is returned.

For the time being I decided that it would make sense to add and delete 
ovpn interfaces via netlink API only.

But there are already discussions about implementing the RTNL 
add/dellink() too.
Therefore I think it makes sense to set dellink to 
unregister_netdevice_queue() in this patch and thus avoid patch 2 at all.


Thanks.

Regards,

quoted

The alternative was to change
default_device_exit_batch/default_device_exit_net to read some new
netdevice flag which would tell if the interface should be killed or
moved to global upon netns exit.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.