linux-kernel - 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: <cf89c31c-7c49-4cd3-a69e-e56207c318de@gmail.com>
Date: Tue, 26 Nov 2024 03:05:47 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>,
 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

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.

--
Sergey