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

On Tue, 28 Jul 2026 12:44:40 +0200, Sabrina Dubroca <[email protected]> wrote:
> 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 :/
>

Yeah, same. I wasn't expecting it to be such a maze either at first.

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

Perfect, then I'll respin the series with the fixes we agreed on.

Thanks!

-- 
Ralf Lici
Mandelbit Srl


_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel