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/+/1434?usp=email
to review the following change.
Change subject: dco_linux: parse stats in PEER_DEL notification if available
......................................................................
dco_linux: parse stats in PEER_DEL notification if available
Upon receiving a PEER_DEL notification, update the peer's statistics
counters if such information is present in the message. This allows
openvpn to avoid issuing a PEER_GET request-reply, eliminating issues
caused by overlapping traffic on the netlink socket when multiple
PEER_DEL notifications are received simultaneously.
Since we cannot assume the kernel module will always send a PEER_DEL
with statistics, introduce a boolean flag to track whether the counters
were already updated or if a PEER_GET request is still required.
Change-Id: I7f3b95cacdf359536e827f176f2b81da4b00f22f
Signed-off-by: Ralf Lici <[email protected]>
---
M src/openvpn/dco.c
M src/openvpn/dco.h
M src/openvpn/dco_freebsd.c
M src/openvpn/dco_freebsd.h
M src/openvpn/dco_linux.c
M src/openvpn/dco_linux.h
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/multi.c
9 files changed, 82 insertions(+), 45 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/34/1434/1
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 7abdad3..3329ff9 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -778,4 +778,10 @@
#endif /* if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) ||
defined(_WIN32) */
}
+bool
+dco_del_peer_stats_updated(dco_context_t *dco)
+{
+ return dco->dco_del_peer_stats_updated;
+}
+
#endif /* defined(ENABLE_DCO) */
diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
index cd6e32a..a81a968 100644
--- a/src/openvpn/dco.h
+++ b/src/openvpn/dco.h
@@ -242,6 +242,16 @@
int dco_get_peer_stats(struct context *c, const bool raise_sigusr1_on_err);
/**
+ * Check if the peer stats were already updated while parsing a PEER_DEL
+ * notification. This is simply a getter for the internal
+ * dco->dco_del_peer_stats_updated field.
+ *
+ * @param dco DCO device context
+ * @return value of dco->dco_del_peer_stats_updated
+ */
+bool dco_del_peer_stats_updated(dco_context_t *dco);
+
+/**
* Retrieve the list of ciphers supported by the current platform
*
* @return list of colon-separated ciphers
@@ -377,6 +387,12 @@
return 0;
}
+static inline bool
+dco_del_peer_stats_updated(dco_context_t *dco)
+{
+ return false;
+}
+
static inline const char *
dco_get_supported_ciphers(void)
{
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index d1ad092..e47ddd2 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -559,13 +559,13 @@
return ret;
}
-static void
+static bool
dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t
*nvl)
{
if (peerid >= m->max_clients || !m->instances[peerid])
{
msg(M_WARN, "dco_update_peer_stat: invalid peer ID %u returned by
kernel", peerid);
- return;
+ return false;
}
struct multi_instance *mi = m->instances[peerid];
@@ -575,6 +575,7 @@
msg(D_DCO_DEBUG, "%s: peer-id %u, dco_read_bytes: " counter_format "
dco_write_bytes: " counter_format,
__func__, peerid, mi->context.c2.dco_read_bytes,
mi->context.c2.dco_write_bytes);
+ return true;
}
int
@@ -620,6 +621,7 @@
{
case OVPN_NOTIF_DEL_PEER:
dco->dco_del_peer_reason = OVPN_DEL_PEER_REASON_EXPIRED;
+ dco->dco_del_peer_stats_updated = false;
if (nvlist_exists_number(nvl, "del_reason"))
{
@@ -642,12 +644,13 @@
if (dco->c->mode == CM_TOP)
{
- dco_update_peer_stat(dco->c->multi,
dco->dco_message_peer_id, bytes);
+ dco->dco_del_peer_stats_updated =
dco_update_peer_stat(dco->c->multi, dco->dco_message_peer_id, bytes);
}
else
{
dco->c->c2.dco_read_bytes = nvlist_get_number(bytes, "in");
dco->c->c2.dco_write_bytes = nvlist_get_number(bytes,
"out");
+ dco->dco_del_peer_stats_updated = true;
}
}
diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h
index 5e2a552..72056b0 100644
--- a/src/openvpn/dco_freebsd.h
+++ b/src/openvpn/dco_freebsd.h
@@ -60,6 +60,7 @@
int dco_message_type;
int dco_message_peer_id;
int dco_del_peer_reason;
+ bool dco_del_peer_stats_updated;
struct sockaddr_storage dco_float_peer_ss;
struct context *c;
diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 2664029..743eb5c 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -49,10 +49,10 @@
#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.
+/* When parsing multiple DEL_PEER notifications that do not include stats,
+ * openvpn tries to request them 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.
@@ -814,9 +814,38 @@
}
}
-static void
-dco_update_peer_stat(struct context_2 *c2, struct nlattr *tb[], uint32_t id)
+static bool
+dco_update_peer_stat(dco_context_t *dco, struct nlattr *tb[], uint32_t id)
{
+ struct context_2 *c2;
+
+ if (dco->ifmode == OVPN_MODE_P2P)
+ {
+ c2 = &dco->c->c2;
+ if (c2->tls_multi->dco_peer_id != id)
+ {
+ return false;
+ }
+ }
+ else
+ {
+ if (id >= dco->c->multi->max_clients)
+ {
+ msg(M_WARN, "%s: received out of bound id %u (max=%u)", __func__,
id,
+ dco->c->multi->max_clients);
+ return false;
+ }
+
+ struct multi_instance *mi = dco->c->multi->instances[id];
+ if (!mi)
+ {
+ msg(M_WARN | M_NOIPREFIX, "%s: received data for a non-existing
peer %u", __func__, id);
+ return false;
+ }
+
+ c2 = &mi->context.c2;
+ }
+
if (tb[OVPN_A_PEER_LINK_RX_BYTES])
{
c2->dco_read_bytes = ovpn_nla_get_uint(tb[OVPN_A_PEER_LINK_RX_BYTES]);
@@ -824,7 +853,8 @@
}
else
{
- msg(M_WARN, "%s: no link RX bytes provided in reply for peer %u",
__func__, id);
+ msg(M_WARN, "%s: no link RX bytes provided for peer %u", __func__, id);
+ return false;
}
if (tb[OVPN_A_PEER_LINK_TX_BYTES])
@@ -834,7 +864,8 @@
}
else
{
- msg(M_WARN, "%s: no link TX bytes provided in reply for peer %u",
__func__, id);
+ msg(M_WARN, "%s: no link TX bytes provided for peer %u", __func__, id);
+ return false;
}
if (tb[OVPN_A_PEER_VPN_RX_BYTES])
@@ -844,7 +875,8 @@
}
else
{
- msg(M_WARN, "%s: no VPN RX bytes provided in reply for peer %u",
__func__, id);
+ msg(M_WARN, "%s: no VPN RX bytes provided for peer %u", __func__, id);
+ return false;
}
if (tb[OVPN_A_PEER_VPN_TX_BYTES])
@@ -854,8 +886,11 @@
}
else
{
- msg(M_WARN, "%s: no VPN TX bytes provided in reply for peer %u",
__func__, id);
+ msg(M_WARN, "%s: no VPN TX bytes provided for peer %u", __func__, id);
+ return false;
}
+
+ return true;
}
static int
@@ -877,40 +912,10 @@
}
uint32_t peer_id = nla_get_u32(tb_peer[OVPN_A_PEER_ID]);
- struct context_2 *c2;
msg(D_DCO_DEBUG | M_NOIPREFIX, "%s: parsing message for peer %u...",
__func__, peer_id);
- if (dco->ifmode == OVPN_MODE_P2P)
- {
- c2 = &dco->c->c2;
- if (c2->tls_multi->dco_peer_id != peer_id)
- {
- return NL_SKIP;
- }
- }
- else
- {
- if (peer_id >= dco->c->multi->max_clients)
- {
- msg(M_WARN, "%s: received out of bound peer_id %u (max=%u)",
__func__, peer_id,
- dco->c->multi->max_clients);
- return NL_SKIP;
- }
-
- struct multi_instance *mi = dco->c->multi->instances[peer_id];
- if (!mi)
- {
- msg(M_WARN | M_NOIPREFIX, "%s: received data for a non-existing
peer %u", __func__, peer_id);
- return NL_SKIP;
- }
-
- c2 = &mi->context.c2;
- }
-
- dco_update_peer_stat(c2, tb_peer, peer_id);
-
- return NL_OK;
+ return dco_update_peer_stat(dco, tb_peer, peer_id) ? NL_OK : NL_SKIP;
}
static bool
@@ -976,6 +981,9 @@
dco->dco_del_peer_reason = reason;
dco->dco_message_type = OVPN_CMD_PEER_DEL_NTF;
+ /* parse stats if available */
+ dco->dco_del_peer_stats_updated = dco_update_peer_stat(dco, dp_attrs,
peerid);
+
return NL_OK;
}
diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
index efd5b27..e275061 100644
--- a/src/openvpn/dco_linux.h
+++ b/src/openvpn/dco_linux.h
@@ -80,6 +80,7 @@
int dco_message_peer_id;
int dco_message_key_id;
int dco_del_peer_reason;
+ bool dco_del_peer_stats_updated;
struct sockaddr_storage dco_float_peer_ss;
} dco_context_t;
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 94f043f..030d353 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -681,6 +681,7 @@
dco->dco_message_peer_id = dco->notif_buf.PeerId;
dco->dco_message_type = dco->notif_buf.Cmd;
dco->dco_del_peer_reason = dco->notif_buf.DelPeerReason;
+ dco->dco_del_peer_stats_updated = false;
dco->dco_float_peer_ss = dco->notif_buf.FloatAddress;
}
else
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 02b8389..599f41d 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -53,6 +53,7 @@
int dco_message_peer_id;
int dco_message_type;
int dco_del_peer_reason;
+ bool dco_del_peer_stats_updated;
struct sockaddr_storage dco_float_peer_ss;
struct context *c;
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 153695c..4c7d3e2 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -510,7 +510,7 @@
static void
setenv_stats(struct multi_context *m, struct context *c)
{
- if (dco_enabled(&m->top.options))
+ if (dco_enabled(&m->top.options) &&
!dco_del_peer_stats_updated(&m->top.c1.tuntap->dco))
{
if (dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, false) < 0)
{
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1434?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: I7f3b95cacdf359536e827f176f2b81da4b00f22f
Gerrit-Change-Number: 1434
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <[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
