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

On Wed, 08 Jul 2026 12:14:08 +0200, Sabrina Dubroca <[email protected]> wrote:
> 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.
>

Right, I wrongly assumed queue_rcu_work was "chain queueing" but it
queues from an RCU callback, not from a worker currently running on
ovpn_wq.

>
> 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 :))
>

Yes, definitely it makes sense.

I went through the ovpn_wq producers:
- ovpn->keepalive_work: disabled from ovpn_net_uninit before peers are
  freed;
- peer->keepalive_work: queued by the scanner, it holds a peer reference
  and can indirectly hit the TCP deferred-delete path through TX
  failure;
- peer->tcp.defer_del_work: queued from TCP RX/TX error paths, each
  successfully queued instance owns a peer reference and peer release
  drops crypto state before dropping the netdev reference;
- sock->tcp_tx_work: queued from TCP write_space, socket teardown
  cancels it;
- key-slot free work/rcu_work: queued from final key-slot put.

So after rtnl_link_unregister has returned, I don't see a peer-owned
producer that can create new key-slot rcu_work without first holding a
peer reference that would have kept unregister_netdevice waiting. Then
flush_workqueue waits for the remaining worker tails that may still be
executing module text. After that, rcu_barrier forces pending rcu_work
callbacks to queue their actual work, and destroy_workqueue drains that
cleanup work before module text goes away.

So the ordering I'd use with queue_rcu_work is:

    ovpn_nl_unregister();
    rtnl_link_unregister(&ovpn_link_ops);
    flush_workqueue(ovpn_wq);
    rcu_barrier();
    destroy_workqueue(ovpn_wq);

Makes sense?

Avoiding work on deleted peers is nice cleanup, but I don't think the
unload safety depends on it because the stronger invariant is that any
peer-owned work that can queue more work either already owns a peer ref
or takes one before queueing.

WDYT?

-- 
Ralf Lici
Mandelbit Srl