[Openvpn-devel,v2,3/4] Ignore OVPN_DEL_PEER_REASON_USERSPACE to avoid race conditions

Message ID 20221227022404.3468137-3-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v2,1/4] Ensure we do not promote a TA_TIMEOUT to a TA_READ event with dco | expand

Commit Message

Arne Schwabe Dec. 27, 2022, 2:24 a.m. UTC
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
 src/openvpn/multi.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)


Gert Doering Dec. 27, 2022, 4:58 p.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

I have not actually seen this particular race (or if I have seen it it
was drowned in lots of other events) - but yeah, to fully ignore this
seems better than "kill peer <x> just because we ended a peer with
the very same peer-id <x> a few seconds ago".

To achieve the "some other program might have sent this" original idea,
one would need to add a sending PID ("we ignore everything with our
own PID"), or some sort of ACK mechanism ("we do not re-assign the
peer-id <x> until we have confirmation from the kernel").  This would
certainly be nice, but for now, ignore it is.

Tested on the ubuntu2004 DCO system (client/server), with no gremlins.

Your patch has been applied to the master branch.

commit 6ad66b0c2950c0d7674a5867085fef8115f61d11 (master)
commit eec054a95820ffa7e548f47078ec0101d6949907 (release/2.6)
Author: Arne Schwabe
Date:   Tue Dec 27 03:24:03 2022 +0100

     Ignore OVPN_DEL_PEER_REASON_USERSPACE to avoid race conditions

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221227022404.3468137-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25820.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>

kind regards,

Gert Doering


diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 6c6385c6e..50d88f19a 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3243,10 +3243,19 @@  process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi,
-            /* This very likely ourselves but might be another process, so
-             * still process it */
-            reason = "ovpn-dco: userspace request";
-            break;
+            /* We assume that is ourselves. UUnfortunately, sometimes these
+             * events happen with enough delay that they can have an order of
+             *
+             * dco_del_peer x
+             * [new client connecting]
+             * dco_new_peer x
+             * event from dco_del_peer arrives.
+             *
+             * if we do not ignore this we get desynced with the kernel
+             * since we assume the peer-id is free again. The other way would
+             * be to send a dco_del_peer again
+             */
+            return;
     /* When kernel already deleted the peer, the socket is no longer