Re: [PATCH ovpn net v5 6/6] ovpn: defer key slot crypto freeing to workqueue

2026-07-08, 09:04:42 +0200, Ralf Lici wrote:
> On Tue, 07 Jul 2026 16:17:11 +0200, Sabrina Dubroca <[email protected]> wrote:
> > 2026-07-06, 13:34:04 +0200, Ralf Lici wrote:
> > > Key slots are released through a kref and the existing release path frees
> > > the AEAD transforms from an RCU callback. That is not safe for all crypto
> > > implementations: crypto_free_aead can sleep, for example when an async or
> > > hardware implementation has teardown work to complete.
> > > 
> > > The AEAD transforms are only used while a key-slot reference is held, so
> > > the final kref put is enough to prove that no transform user remains.
> > > Queue the final crypto teardown on the ovpn workqueue, where sleeping is
> > > allowed, and keep the slot memory itself RCU-freed after the transforms
> > > have been released.
> > > 
> > > ovpn_crypto_config_get was the remaining lockless reader of transform
> >
> > nit: encrypt/decrypt is also lockless, the difference is only the
> > refcount, no?
> >
> 
> Correct, I meant the remaining reader of transform state without holding
> a key-slot reference. I'll reword that.
> 
> > > state through ovpn_aead_crypto_alg. Make it take a key-slot reference
> > > before reading that state, so the transform lifetime is consistently tied
> > > to key-slot references. The previous patch drains ovpn_wq during module
> >
> > Using rcu_work/queue_rcu_work (see commit 6624bba469a3 ("macsec: use
> > rcu_work to defer RX SA crypto cleanup out of softirq")) would avoid
> > some of the refactoring of the destroy helpers, and the (kind of weird
> > and ugly IMO) refcount dance in ovpn_crypto_config_get (with a fairly
> > big risk of "well it's dumb to take a refcount here, we could just do
> > everything under RCU").
> >
> 
> One thing I am not fully sure about in this case is the module-exit
> ordering. If key-slot release uses queue_rcu_work, the work item is not
> queued immediately. The final put first schedules an RCU callback, and
> that callback later queues the actual work on ovpn_wq, right?

AFAIU, yes.

> Does that mean ovpn_cleanup would need a different ordering from the
> current destroy_workqueue; rcu_barrier shape?
> 
> In particular, with this series ovpn_wq also runs ordinary ovpn work
> items, such as the TCP deferred-delete worker and keepalive work. Those
> can drop peer references, and the final peer put can release the crypto
> state and queue a key-slot rcu_work. So if we only move rcu_barrier
> before destroy_workqueue, maybe an already pending/running ovpn worker
> could still queue a new rcu_work after the barrier has returned.

Right, that's... a bit hairy.

> Would the safe shape be something like:
> 
>     drain_workqueue(ovpn_wq);
>     rcu_barrier();
>     destroy_workqueue(ovpn_wq);
> 
> so that ordinary ovpn work is drained first, including chained work
> queued by already running ovpn workers, then pending rcu_work callbacks
> are forced to queue their actual work, and finally the workqueue drains
> that cleanup work before being destroyed?

I'm not sure. The comment for drain_workqueue() says:

 * Wait until the workqueue becomes empty.  While draining is in progress,
 * only chain queueing is allowed.  IOW, only currently pending or running
 * work items on @wq can queue further work items on it.  [...]

So any attempt to queue from other contexts will fail (and actually
trigger a WARN it seems, __queue_work -> __WQ_DRAINING), for example
from TCP. Even without the warning, for things like keepalive or
tcp_tx, it shouldn't matter, since we're already cleaning up. Maybe
also defer_del_work, since the peer should already be going away with
its netdevice. But it we can't queue key slot freeing, it's going to
leak.


Would flush_workqueue() be enough? By the time rtnl_link_unregister()
returns, ndo_uninit() has been called for all ovpn devices
(__rtnl_kill_links() -> unregister_netdevice_many_notify() ->
->ndo_uninit()) so ovpn_peers_free() has removed all the peers from
the hashtable/p2p slot.

ovpn_peer_keepalive_work_{p2p,mp} should probably copy the "prevent
double remove" check from ovpn_peer_remove() as "don't do anything for
already deleted peers". tcp_tx_work can queue defer_del_work but the
same check could be used. With this, after rtnl_link_unregister()
returns, there should be no work capable of requeueing work on
ovpn_wq, so no more keepalive_work or defer_del_work running once
flush_workqueue() returns, or maybe only "empty" ones that will just
return immediately without doing anything.

The final destroy_workqueue() will take care of any keyslot
cleanup that has been queued after flush_workqueue() started (whether
it was while flush_workqueue() was running, or during the
rcu_barrier()).

Am I making any sense? (it's quite possible I need another 3 cups of
coffee for this type of reasoning :))

-- 
Sabrina