Re: [PATCH net-next v7 03/25] net: introduce OpenVPN Data Channel Offload (ovpn)

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

Hello Antonio, Kuniyuki,

On 20.09.2024 12:46, Antonio Quartulli wrote:
> 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
>>> Hi Kuniyuki and thank you for chiming in.
>>>
>>> On 19/09/2024 07:52, Kuniyuki Iwashima wrote:
>>>> From: Antonio Quartulli <antonio@openvpn.net>
>>>> Date: Tue, 17 Sep 2024 03:07:12 +0200
>>>>> +/* 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.

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.

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.

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