diff mbox series

[Openvpn-devel,v1] tunnel_server(): close correct inotify fd

Message ID 20260128110425.24350-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v1] tunnel_server(): close correct inotify fd | expand

Commit Message

Gert Doering Jan. 28, 2026, 11:04 a.m. UTC 
On a full SIGUSR1 restart of a p2mp server compiled with
--enable-async-push, tunnel_server() will try to close and reopen
the "inotify" control file descriptor.  For whatever reason, the
original code referenced the wrong context, always closing fd 0.

As a consequence of this, on the second SIGUSR1 restart, the server
will close() the first active socket file descriptor, and if there
are active DCO clients, the resulting event confusion will lead to
an ASSERT(!mi->halt).

Fix by closing the correct FD. Add logging.

Github: fixes OpenVPN/openvpn#966

Change-Id: Iabc117848ad7b67d240c392f1a6aa2d7531fd5bb
Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1497
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1497
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering Jan. 28, 2026, 1:28 p.m. UTC | #1 
This was an... interesting chase.  It started out by testing the fix for
a management-interface weirdness (commit 4bf05d487c) and all of a sudden
I had the server crash with ASSERT(!mi->halt) on me... which turned out
to be a bug in really ancient code... (commit 619c3e9a0 moved around the
inotify stuff in the context of multisocket, but did not introduce the
confusion on "top" contexts - that was already in the initial inotify
commit 0d1a75bfe).  Full details in the github issue...

For now, this patch has been applied to the master branch (to progress
towards 2.7_rc6).  I will come back here to see which branches need the
fix, or whether different "top context" handling actually made it do the
right thing 10 years ago...

commit 5521872f80313060b659c27c55df2a6fdb74ec7a
Author: Gert Doering
Date:   Wed Jan 28 12:04:19 2026 +0100

     tunnel_server(): close correct inotify fd

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1497
     Message-Id: <20260128110425.24350-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg35478.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Jan. 28, 2026, 2:48 p.m. UTC | #2 
Hi,

On Wed, Jan 28, 2026 at 02:28:11PM +0100, Gert Doering wrote:
> For now, this patch has been applied to the master branch (to progress
> towards 2.7_rc6).  I will come back here to see which branches need the
> fix, or whether different "top context" handling actually made it do the
> right thing 10 years ago...

As it turns out, the code in 2.6 and earlier does not have a "multi.c", but
instead it has "mudp.c" and "mtcp.c" for UDP and TCP servers, respectively.

The code is the same, the "close(0)" is the same, but the patch cannot be
"just cherrypicked", so I've put a 2.6 patch to gerrit

   http://gerrit.openvpn.net/c/openvpn/+/1499

gert
diff mbox series

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 2af49d2..8f903a4 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -4244,7 +4244,9 @@ 
     tunnel_server_loop(&multi);
 
 #ifdef ENABLE_ASYNC_PUSH
-    close(top->c2.inotify_fd);
+    msg(D_LOW, "%s: close multi.top.c2.inotify_fd (%d)",
+                __func__, multi.top.c2.inotify_fd );
+    close(multi.top.c2.inotify_fd);
 #endif
 
     /* shut down management interface */