netdev - Re: [PATCH net-next v11 12/23] ovpn: implement TCP transport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <08044f81-3b57-4abc-b04b-08de0f4735a6@openvpn.net>
Date: Tue, 26 Nov 2024 09:51:12 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Sabrina Dubroca <sd@...asysnail.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, Andrew Lunn <andrew@...n.ch>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 12/23] ovpn: implement TCP transport

On 26/11/2024 02:05, Sergey Ryazanov wrote:
> Hi Antonio,
> 
> the question was addressed to Sabrina, but since I've already touched 
> this topic in the another patch, let me put my 2c here.
> 
> On 16.11.2024 02:33, Antonio Quartulli wrote:
>> On 31/10/2024 16:25, Sabrina Dubroca wrote:
>>> 2024-10-29, 11:47:25 +0100, Antonio Quartulli wrote:
>>>> +static void ovpn_socket_release_work(struct work_struct *work)
>>>> +{
>>>> +    struct ovpn_socket *sock = container_of(work, struct 
>>>> ovpn_socket, work);
>>>> +
>>>> +    ovpn_socket_detach(sock->sock);
>>>> +    kfree_rcu(sock, rcu);
>>>> +}
>>>> +
>>>> +static void ovpn_socket_schedule_release(struct ovpn_socket *sock)
>>>> +{
>>>> +    INIT_WORK(&sock->work, ovpn_socket_release_work);
>>>> +    schedule_work(&sock->work);
>>>
>>> How does module unloading know that it has to wait for this work to
>>> complete? Will ovpn_cleanup get stuck until some refcount gets
>>> released by this work?
>>
>> No, we have no such mechanism.
>> Any idea how other modules do?
>>
>> Actually this makes me wonder how module unloading coordinates with 
>> the code being executed. Unload may happen at any time - how do we 
>> prevent killing the code in the middle of something (regardless of 
>> scheduled workers)?
> 
> Right question! There is a workqueue flushing API intended for 
> synchronization with work(s) execution.
> 
> Here, the system workqueue was used, so technically a 
> flush_scheduled_work() call somewhere in the module_exit handler would 
> be enough.
> 
> On another hand, flushing the system workqueue considered a not so good 
> practice. It's recommended to use a local workqueue. You can find a good 
> example of switching from the system to a local workqueue in 
> cc271ab86606 ("wwan_hwsim: Avoid flush_scheduled_work() usage").
> 
> 
> And if the workqueue is definitely empty at a time of module unloading, 
> e.g. due to flushing on netdev removing, there no requirement to flush 
> it again.

ACK. I wanted to avoid using a local workqueue, but if we have pending 
work that needs flushing I indeed see no other way.

Regards,

> 
> -- 
> Sergey

-- 
Antonio Quartulli
OpenVPN Inc.