Message-ID: <0c5daaf7-24b3-42ed-935e-05dd7d69edd9@openvpn.net>
Date: Mon, 23 Sep 2024 14:51:38 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: andrew@...n.ch, antony.antony@...unet.com, edumazet@...gle.com,
 kuba@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com,
 sd@...asysnail.net, steffen.klassert@...unet.com
Subject: Re: [PATCH net-next v7 03/25] net: introduce OpenVPN Data Channel
 Offload (ovpn)

On 22/09/2024 22:51, Sergey Ryazanov wrote:
> 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@...nvpn.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@...nvpn.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.

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,

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

Powered by blists - more mailing lists