Promptly close the netcmd_semaphore handle after use

PATCH In review Selva Nair 2 participants 5 messages Jun 14, 2016 – Jul 11, 2016

Share & tools

Selva Nair

author

2016-06-14 02:34 UTC #0 lore raw

PATCH 1/1 Promptly close the netcmd_semaphore handle after use

If more than one openvpn processes are running and one aborts without releasing the semaphore, subsequent processes fail to get a lock for the semaphore. This may be avoided by not keeping open handles to the semaphore so that Windows can destroy it when no open handles remain.

Signed-off-by Selva Nair <[email protected]...>

---
 src/openvpn/win32.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

src/openvpn/win32.c +6 -1

index 8a2f957..ad721cd 100644
@@ -105,7 +105,6 @@ init_win32 (void)
     }
   window_title_clear (&window_title);
   win32_signal_clear (&win32_signal);
-  netcmd_semaphore_init ();
 }
 
 void

@@ -751,6 +750,10 @@ void
 netcmd_semaphore_lock (void)
 {
   const int timeout_seconds = 600;
+
+  if (!netcmd_semaphore.hand)
+    netcmd_semaphore_init ();
+
   if (!semaphore_lock (&netcmd_semaphore, timeout_seconds * 1000))
     msg (M_FATAL, "Cannot lock net command semaphore"); 
 }

@@ -759,6 +762,8 @@ void
 netcmd_semaphore_release (void)
 {
   semaphore_release (&netcmd_semaphore);
+  /* netcmd_semaphore has max count of 1 - safe to close after release */
+  semaphore_close (&netcmd_semaphore);
 }
 
 /*

Signature · 1 line

1.7.10.4

Gert Doering

2016-06-14 07:13 UTC #1 lore raw

Hi,

src/openvpn/win32.c void · line 767

Quoted 7 lines · Selva Nair

src/openvpn/win32.c +2 -0

@@ -759,6 +762,8 @@ void
 netcmd_semaphore_release (void)
 {
   semaphore_release (&netcmd_semaphore);
+  /* netcmd_semaphore has max count of 1 - safe to close after release */
+  semaphore_close (&netcmd_semaphore);
 }

This should work, but it is more "aggressive" than I had in mind - releasing the semaphore afer every single call, so when setting up lots of routes with netsh.exe, we might end up opening and closing the semaphore "dozens" of times.

How expensive / slow is this? Has this significant impact?

My idea was to close the semaphore when the initialization phase is over, like in init.c, initialization_sequence_completed(), in the existing WIN32 block

#ifdef WIN32
netcmd_semaphore_close(); /* release handles */
fork_register_dns_action (c->c1.tuntap);
#endif

or something like that. Keeping your change to the "lock" call so it is then reopened at tunnel teardown, but is not open while openvpn is running in steady state.

But I'm open to "semaphores are cheap, we can just open/lock/release/close it on every call" arguments :-)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Selva Nair

author

2016-06-15 03:40 UTC #2 lore raw

Hi,

Quoted 19 lines · Gert Doering

src/openvpn/win32.c void · line 765
Quoted 5 lines · Selva Nair
src/openvpn/win32.c +1 -0
@@ -759,6 +762,8 @@ void
 netcmd_semaphore_release (void)
 {
   semaphore_release (&netcmd_semaphore);
+  /* netcmd_semaphore has max count of 1 - safe to close after release
*/
src/openvpn/win32.c void · line 767
Quoted 2 lines
src/openvpn/win32.c +1 -0
+  semaphore_close (&netcmd_semaphore);
 }
This should work, but it is more "aggressive" than I had in mind - releasing
the semaphore afer every single call, so when setting up lots of routes with
netsh.exe, we might end up opening and closing the semaphore "dozens" of times.

Quoted 2 lines

How expensive / slow is this? Has this significant impact?

My idea was to close the semaphore when the initialization phase is over,

Quoted 12 lines

like in init.c, initialization_sequence_completed(), in the existing WIN32 block
#ifdef WIN32
netcmd_semaphore_close(); /* release handles */
fork_register_dns_action (c->c1.tuntap);
#endif
or something like that. Keeping your change to the "lock" call so it is then reopened at tunnel teardown, but is not open while openvpn is running in steady state.

If we go this route we'll also need a netcmd_semaphore_close() when the SIGUSR1 loop exits, as a restart before initialization_completed could wait at management-hold with an open handle.

Or a SIGHUP will open the semaphore to delete routes and the restart could end up in management hold.

Quoted 3 lines

But I'm open to "semaphores are cheap, we can just open/lock/release/close it on every call" arguments :-)

Very unlikely to be a drag on performance -- I think createprocess will be where time is spent [*].

That said, calling semaphore_close() inside netcmd_semaphore_unlock() is not a style to be proud of. So I'm kind of inclined to do a version 2 along the lines you suggested but some testing will be needed to get it right.

Selva

P.S:
[*] FWIW, I did a timing test: Run using a modified openvpn that just does openvpn_execve of a route add and route delete inside a for loop. The times below are for a loop count of 100.

No semaphore: 2.45 to 2.49 seconds
With semaphore lock/unlock : 2.52 to 2.53 seconds With semaphore closed after reach unlock: 2.52 to 2.54 seconds

(Not enough statistics to give a precise number, but the overhead of using the semaphore is small and the open/close appears to make little difference). I'm surprised that it takes 2.5 seconds to add and delete a route just 100 times. Never use --route-method exe

Note that all times are real time, not CPU time. And the times are for just the for loop and scales linearly -- tested with 200 and 300 counts as well.

Selva Nair

author

2016-07-06 00:06 UTC #3 lore raw

Quoted 7 lines · Selva Nair

Quoted 6 lines
But I'm open to "semaphores are cheap, we can just open/lock/release/ close
it on every call" arguments :-)

Quoted 7 lines

Very unlikely to be a drag on performance -- I think createprocess will be where time is spent [*].
That said, calling semaphore_close() inside netcmd_semaphore_unlock() is not a style to be proud of. So I'm kind of inclined to do a version 2 along the lines you suggested but some testing will be needed to get it right.

After looking into this further, I find redoing the patch along those lines is not ideal. A semaphore_close() has to be inserted before any possible long waits such as management-hold, pull-filter reject, or any other restart loops, which is going to be ugly and fragile.

The cleanest way is to close the semaphore soon after releasing it. Version 1 does that and I do not think there is any performance penalty involved. Logically, a more correct approach may be to replace all calls to netcmd_semaphore_release()
by netcmd_semaphore_close() but that involves more pervasive edits.

Having said that, I'm open to other suggestions.

Selva

Gert Doering

2016-07-11 18:13 UTC #4 lore raw

In reply to Selva Nair

ACK. Reviewed the patch before - performance impact is not high (compared to exec()'ing netsh.exe), and I can see the argument that the approach I suggested would be changing too many different places to be more "clean". Compile-tested, not run.

Your patch has been applied to the master and release/2.3 branch (bugfix).

commit 6aa4c9091300f62fae0bf7a9198de0edd2d8b7c7 (master) commit 13f3526a9b05bed63c50c93c5903b0cde0dcc7f4 (release/2.3)

Author: Selva Nair
Date:   Mon Jun 13 22:34:49 2016 -0400

     Promptly close the netcmd_semaphore handle after use

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11919
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering