[M] Change in openvpn[master]: dco_linux: read multicast notifications on a dedicated netlink socket

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/+/1737?usp=email

to review the following change.


Change subject: dco_linux: read multicast notifications on a dedicated netlink socket
......................................................................

dco_linux: read multicast notifications on a dedicated netlink socket

ovpn-dco shared a single netlink socket for both synchronous
request/reply transactions (peer create/delete/get, key operations,
stats) and the asynchronous multicast notification group (peer
del/float, key swap). Because every ovpn_nl_msg_send() drains its reply
with nl_recvmsgs() on that same socket, a notification queued by the
kernel could be dispatched re-entrantly in the middle of an unrelated
request/reply: the NL_CB_VALID handler runs ovpn_handle_msg() ->
multi_process_incoming_dco() -> multi_close_instance() while the caller
is still walking an instance collection. For example multi_print_status()
refreshes stats with dco_get_peer_stats_multi() and then iterates
m->hash, and multi_delete_dup() iterates m->instances[] while closing
duplicates; an instance freed underneath either iterator becomes a
use-after-free and the server crashes (observed under mass simultaneous
client reconnects).

Subscribe the multicast group on a dedicated socket (nl_sock_notify) and
read it only from dco_read_and_process(), the top-level event-loop point
where closing an instance is safe. The request/reply socket is no longer
a member of the group, so parsing a command or stats reply can never
dispatch a notification. The event loop now monitors the notification
socket; the request/reply socket keeps being drained synchronously by
ovpn_nl_msg_send().

Change-Id: Ia7894fc360b12c5a0f63fad2abb9bbb1ad9c2bd5
GitHub: closes OpenVPN/openvpn#1067
Signed-off-by: Antonio Quartulli <[email protected]>
---
M src/openvpn/dco_linux.c
M src/openvpn/dco_linux.h
2 files changed, 74 insertions(+), 11 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/37/1737/1

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 40746bd..7a0f620 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -134,12 +134,8 @@
 }
 
 static int
-ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix)
+ovpn_nl_recvmsgs_report(int ret, const char *prefix)
 {
-    __is_locked = true;
-    int ret = nl_recvmsgs(dco->nl_sock, dco->nl_cb);
-    __is_locked = false;
-
     switch (ret)
     {
         case -NLE_INTR:
@@ -174,6 +170,16 @@
     return ret;
 }
 
+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);
+}
+
 /**
  * Send a prepared netlink message.
  *
@@ -398,9 +404,11 @@
     }
 
     /* Register for ovpn-dco specific multicast messages that the kernel may
-     * send
+     * send. These are subscribed on the dedicated notification socket only, so
+     * that they are never delivered while a request/reply transaction on
+     * dco->nl_sock is being parsed.
      */
-    int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id);
+    int ret = nl_socket_add_membership(dco->nl_sock_notify, dco->ovpn_dco_mcast_id);
     if (ret)
     {
         msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret);
@@ -449,17 +457,49 @@
     nl_cb_set(dco->nl_cb, NL_CB_ACK, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status);
     nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco);
 
+    /* Set up the dedicated multicast notification socket. It shares the message
+     * handler (ovpn_handle_msg) with the request/reply socket, but is read only
+     * from dco_read_and_process() in the event loop, so notifications never
+     * fire while a request/reply transaction is being parsed. */
+    dco->nl_sock_notify = nl_socket_alloc();
+    if (!dco->nl_sock_notify)
+    {
+        msg(M_FATAL, "Cannot create netlink notification socket");
+    }
+
+    ret = genl_connect(dco->nl_sock_notify);
+    if (ret)
+    {
+        msg(M_FATAL, "Cannot connect to generic netlink (notify): %s", nl_geterror(ret));
+    }
+
+    set_cloexec(nl_socket_get_fd(dco->nl_sock_notify));
+    set_nonblock(nl_socket_get_fd(dco->nl_sock_notify));
+
+    dco->nl_cb_notify = nl_cb_alloc(NL_CB_DEFAULT);
+    if (!dco->nl_cb_notify)
+    {
+        msg(M_FATAL, "failed to allocate netlink notification callback");
+    }
+
+    nl_socket_set_cb(dco->nl_sock_notify, dco->nl_cb_notify);
+    nl_cb_err(dco->nl_cb_notify, NL_CB_CUSTOM, ovpn_nl_cb_error, &dco->status);
+    nl_cb_set(dco->nl_cb_notify, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco);
+
     ovpn_dco_register(dco);
 
     /* The async PACKET messages confuse libnl and it will drop them with
      * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence
-     * number check */
+     * number check. Multicast notifications are unsolicited and likewise carry
+     * no matching sequence number, so disable the check on both sockets. */
     nl_socket_disable_seq_check(dco->nl_sock);
+    nl_socket_disable_seq_check(dco->nl_sock_notify);
 
     /* nl library sets the buffer size to 32k/32k by default which is sometimes
      * overrun with very fast connecting/disconnecting clients.
      * TODO: fix this in a better and more reliable way */
     ASSERT(!nl_socket_set_buffer_size(dco->nl_sock, 1024 * 1024, 1024 * 1024));
+    ASSERT(!nl_socket_set_buffer_size(dco->nl_sock_notify, 1024 * 1024, 1024 * 1024));
 }
 
 bool
@@ -495,8 +535,12 @@
     nl_socket_free(dco->nl_sock);
     dco->nl_sock = NULL;
 
+    nl_socket_free(dco->nl_sock_notify);
+    dco->nl_sock_notify = NULL;
+
     /* Decrease reference count */
     nl_cb_put(dco->nl_cb);
+    nl_cb_put(dco->nl_cb_notify);
 
     CLEAR(dco);
 }
@@ -1164,7 +1208,13 @@
 {
     msg(D_DCO_DEBUG, __func__);
 
-    return ovpn_nl_recvmsgs(dco, __func__);
+    /* Drain the multicast notification socket. This is the only place where
+     * asynchronous notifications (peer del/float, key swap) are processed, so
+     * the multi_close_instance() they may trigger happens at a safe point in
+     * the event loop rather than re-entrantly inside a request/reply. */
+    int ret = nl_recvmsgs(dco->nl_sock_notify, dco->nl_cb_notify);
+
+    return ovpn_nl_recvmsgs_report(ret, __func__);
 }
 
 static int
@@ -1326,9 +1376,12 @@
 void
 dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
 {
-    if (dco && dco->nl_sock)
+    if (dco && dco->nl_sock_notify)
     {
-        event_ctl(es, nl_socket_get_fd(dco->nl_sock), EVENT_READ, arg);
+        /* Only the notification socket is monitored by the event loop. The
+         * request/reply socket (dco->nl_sock) is drained synchronously by
+         * ovpn_nl_msg_send(). */
+        event_ctl(es, nl_socket_get_fd(dco->nl_sock_notify), EVENT_READ, arg);
     }
 }
 
diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
index e3e4824..c487581 100644
--- a/src/openvpn/dco_linux.h
+++ b/src/openvpn/dco_linux.h
@@ -62,8 +62,18 @@
 
 typedef struct
 {
+    /* socket used for synchronous request/reply transactions (commands and
+     * stats queries). It is NOT subscribed to the multicast group, so its
+     * recvmsgs never dispatch an asynchronous notification. */
     struct nl_sock *nl_sock;
     struct nl_cb *nl_cb;
+    /* socket dedicated to multicast notifications (peer del/float, key swap).
+     * It is read only at the top-level dco_read_and_process() point, where it
+     * is safe to close instances. Keeping it separate from nl_sock prevents
+     * notifications from being processed re-entrantly while a request/reply is
+     * in flight. */
+    struct nl_sock *nl_sock_notify;
+    struct nl_cb *nl_cb_notify;
     int status;
 
     struct context *c;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1737?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: Ia7894fc360b12c5a0f63fad2abb9bbb1ad9c2bd5
Gerrit-Change-Number: 1737
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