[S] Change in openvpn[master]: dco_linux: drop the now-redundant __is_locked re-entrancy guard

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1738?usp=email

to review the following change.


Change subject: dco_linux: drop the now-redundant __is_locked re-entrancy guard
......................................................................

dco_linux: drop the now-redundant __is_locked re-entrancy guard

__is_locked existed only to stop setenv_stats() from issuing a GET_PEER
request/reply while the shared socket was still being drained for a batch
of notifications, which could fail with NLE_BUSY/NLE_NOMEM or re-enter
nl_recvmsgs() on the busy socket. Now that notifications are read on a
dedicated socket and GET_PEER goes to the request/reply socket, the two
never share an nl_recvmsgs() call: the request/reply socket no longer
dispatches notifications, so multi_process_incoming_dco() (the only path
that reaches dco_get_peer() during message parsing) never runs while that
socket is in flight, and the flag is always false at its check. Remove
the flag, its set/clear and the early return.

Change-Id: I46a9ef71359f42395d69cf899c948014fb72f65a
Signed-off-by: Antonio Quartulli <[email protected]>
---
M src/openvpn/dco_linux.c
1 file changed, 4 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/38/1738/1

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 7a0f620..199a11b 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -49,16 +49,6 @@
 #include <netlink/genl/family.h>
 #include <netlink/genl/ctrl.h>
 
-/* When parsing multiple DEL_PEER notifications, openvpn tries to request stats
- * for each DEL_PEER message (see setenv_stats). This triggers a GET_PEER
- * request-reply while we are still parsing the rest of the initial
- * notifications, which can lead to NLE_BUSY or even NLE_NOMEM.
- *
- * This basic lock ensures we don't bite our own tail by issuing a dco_get_peer
- * while still busy receiving and parsing other messages.
- */
-static bool __is_locked = false;
-
 /* libnl < 3.5.0 does not set the NLA_F_NESTED on its own, therefore we
  * have to explicitly do it to prevent the kernel from failing upon
  * parsing of the message
@@ -170,12 +160,14 @@
     return ret;
 }
 
+/* Drain the request/reply socket. Used to read command/stats replies. This
+ * socket is never subscribed to the multicast group, so it cannot deliver an
+ * asynchronous notification: a reply being parsed here can therefore never
+ * trigger an instance close or a re-entrant request on the same socket. */
 static int
 ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix)
 {
-    __is_locked = true;
     int ret = nl_recvmsgs(dco->nl_sock, dco->nl_cb);
-    __is_locked = false;
 
     return ovpn_nl_recvmsgs_report(ret, prefix);
 }
@@ -1222,12 +1214,6 @@
 {
     ASSERT(dco);
 
-    if (__is_locked)
-    {
-        msg(D_DCO_DEBUG, "%s: cannot request peer stats while parsing other messages", __func__);
-        return 0;
-    }
-
     /* peer_id == -1 means "dump all peers", but this is allowed in MP mode only.
      * If it happens in P2P mode it means that the DCO peer was deleted and we
      * can simply bail out

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1738?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46a9ef71359f42395d69cf899c948014fb72f65a
Gerrit-Change-Number: 1738
Gerrit-PatchSet: 1
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

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