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

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?

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.

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?

-- 
Ralf Lici
Mandelbit Srl