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

2026-07-08, 17:30:49 +0200, Ralf Lici wrote:
> On Wed, 08 Jul 2026 12:14:08 +0200, Sabrina Dubroca <[email protected]> wrote:
> > 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

All those objects referencing each other make it really hard to reason
about correctness :/

> 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?

All this looks correct. Thanks.

-- 
Sabrina