On 22/09/2024 22:51, Sergey Ryazanov wrote:

Hello Antonio, Kuniyuki,

On 20.09.2024 12:46, Antonio Quartulli wrote:

quoted

Hi,

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

quoted

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.

quoted

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.

I should make a confession :) It was me who proposed and pushed the idea 
of the RTNL ops removing. I was too concerned about uselessness of 
addlink operation so I did not clearly mention that dellink is useful 
operation. Especially when it comes to namespace destruction. My bad.

It helped getting where we are now :)

So yeah, providing the dellink operation make sense for namespace 
destruction handling and for user to manually cleanup reminding network 
interfaces after a forceful user application killing or crash.

For this specific case (i.e. crash) I am planning to add a netlink 
notifier that detects when the process having created the interface goes 
away and then kill the interface from within the kernel.

This way we have some sort of self cleanup and avoid leaving the system 
in a bogus state. (For those specific use cases where you want to create 
a "persistent" interface, I think we will provide a flag. But this is 
for a later patch..)


Cheers,

quoted

quoted

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.

-- 
Sergey

-- 
Antonio Quartulli
OpenVPN Inc.