Skip to content

Commit 48a571b

Browse files
author
Stefan Baranoff
committed
dco_linux: ensure all client disconnects are processed
Under Linux DCO , netlink can return multiple messages in one call. The receive calls in use call a callback once per message. Previously each callback call would update flags on a global `dco` object. This was based on two assumptions: 1. Only one code path would lead to a netlink receive call that would result in finding out a client disconnected. This path was set to handle the flags properly. 2. Each receive call would only trigger one callback that was setting the single set of flags on that global object. Both of these are false assumptions. Any receive call to netlink (e.g. to get stats) can result in receiving "unrelated" messages, like client disconnects. A previous patch at least unified the callbacks to handle running the proper callback code, but that doesn't resolve the issue that the stats gathering path does not have the code to handle a client disconnect. Multiple messages can be processed in a single receive call, so if two or more devices disconnect then a single read call will result in only the last of the messages setting flags (overwriting the other callback values set previously) and only that device is processed for disconnection in user space. This patch resolves both by handling the disconnect logic directly in the netlink callback rather than setting flags and handling it later.
1 parent e86ce85 commit 48a571b

File tree

4 files changed

+79
-37
lines changed

4 files changed

+79
-37
lines changed

‎src/openvpn/dco_linux.c‎

Lines changed: 70 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -984,23 +984,87 @@ ovpn_handle_msg(struct nl_msg *msg, void *arg)
984984
return NL_SKIP;
985985
}
986986
int reason = nla_get_u8(dp_attrs[OVPN_DEL_PEER_ATTR_REASON]);
987-
unsigned int peerid = nla_get_u32(dp_attrs[OVPN_DEL_PEER_ATTR_PEER_ID]);
987+
unsigned int peer_id = nla_get_u32(dp_attrs[OVPN_DEL_PEER_ATTR_PEER_ID]);
988988

989989
msg(D_DCO_DEBUG, "ovpn-dco: received CMD_DEL_PEER, ifindex: %d, peer-id %d, reason: %d",
990-
ifindex, peerid, reason);
991-
dco->dco_message_peer_id = peerid;
992-
dco->dco_del_peer_reason = reason;
993-
dco->dco_message_type = OVPN_CMD_DEL_PEER;
990+
ifindex, peer_id, reason);
994991

995-
break;
992+
if (dco->c->mode == CM_TOP)
993+
{
994+
struct multi_context *m = dco->c->multi;
995+
struct multi_instance *mi = NULL;
996+
997+
/* no peer-specific message delivered -> nothing to process.
998+
* bail out right away
999+
*/
1000+
if (peer_id < 0)
1001+
{
1002+
return NL_SKIP;
1003+
}
1004+
1005+
if ((peer_id < m->max_clients) && (m->instances[peer_id]))
1006+
{
1007+
mi = m->instances[peer_id];
1008+
set_prefix(mi);
1009+
dco->dco_del_peer_reason = reason;
1010+
process_incoming_del_peer(m, mi, dco);
1011+
clear_prefix();
1012+
}
1013+
else
1014+
{
1015+
int msglevel = D_DCO;
1016+
if (reason == OVPN_DEL_PEER_REASON_USERSPACE)
1017+
{
1018+
/* we receive OVPN_CMD_DEL_PEER message with reason USERSPACE
1019+
* after we kill the peer ourselves. This peer may have already
1020+
* been deleted, so we end up here.
1021+
* In this case, print the following debug message with DCO_DEBUG
1022+
* level only to avoid polluting the standard DCO level with this
1023+
* harmless event.
1024+
*/
1025+
msglevel = D_DCO_DEBUG;
1026+
}
1027+
msg(msglevel, "Received DCO message for unknown peer-id: %d, "
1028+
"type %d, del_peer_reason %d", peer_id, OVPN_CMD_DEL_PEER,
1029+
reason);
1030+
}
1031+
1032+
}
1033+
else
1034+
{
1035+
/* FreeBSD currently sends us removal notifcation with the old peer-id in
1036+
* p2p mode with the ping timeout reason, so ignore that one to not shoot
1037+
* ourselves in the foot and removing the just established session */
1038+
if (peer_id != dco->c->c2.tls_multi->dco_peer_id)
1039+
{
1040+
msg(D_DCO_DEBUG, "%s: received message for mismatching peer-id %d, "
1041+
"expected %d", __func__, peer_id,
1042+
dco->c->c2.tls_multi->dco_peer_id);
1043+
return NL_SKIP;
1044+
}
1045+
if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED)
1046+
{
1047+
msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
1048+
"%d", __func__, peer_id);
1049+
trigger_ping_timeout_signal(dco->c);
1050+
return NL_SKIP;
1051+
}
1052+
}
9961053
}
1054+
break;
9971055

9981056
default:
9991057
msg(D_DCO, "ovpn-dco: received unknown command: %d", gnlh->cmd);
10001058
dco->dco_message_type = 0;
10011059
return NL_SKIP;
10021060
}
10031061

1062+
dco->dco_message_type = 0;
1063+
dco->dco_message_peer_id = -1;
1064+
dco->dco_del_peer_reason = -1;
1065+
dco->dco_read_bytes = 0;
1066+
dco->dco_write_bytes = 0;
1067+
10041068
return NL_OK;
10051069
}
10061070

‎src/openvpn/forward.c‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,16 +1290,6 @@ process_incoming_dco(struct context *c)
12901290

12911291
switch (dco->dco_message_type)
12921292
{
1293-
case OVPN_CMD_DEL_PEER:
1294-
if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED)
1295-
{
1296-
msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
1297-
"%d", __func__, dco->dco_message_peer_id);
1298-
trigger_ping_timeout_signal(c);
1299-
return;
1300-
}
1301-
break;
1302-
13031293
case OVPN_CMD_SWAP_KEYS:
13041294
msg(D_DCO_DEBUG, "%s: received key rotation notification for peer-id %d",
13051295
__func__, dco->dco_message_peer_id);

‎src/openvpn/multi.c‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3244,7 +3244,7 @@ multi_signal_instance(struct multi_context *m, struct multi_instance *mi, const
32443244
#endif
32453245

32463246
#if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD))
3247-
static void
3247+
void
32483248
process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi,
32493249
dco_context_t *dco)
32503250
{
@@ -3311,9 +3311,9 @@ multi_process_incoming_dco(struct multi_context *m)
33113311
{
33123312
mi = m->instances[peer_id];
33133313
set_prefix(mi);
3314-
if (dco->dco_message_type == OVPN_CMD_DEL_PEER)
3314+
if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS)
33153315
{
3316-
process_incoming_del_peer(m, mi, dco);
3316+
tls_session_soft_reset(mi->context.c2.tls_multi);
33173317
}
33183318
#if defined(TARGET_FREEBSD)
33193319
else if (dco->dco_message_type == OVPN_CMD_FLOAT_PEER)
@@ -3326,28 +3326,11 @@ multi_process_incoming_dco(struct multi_context *m)
33263326
CLEAR(dco->dco_float_peer_ss);
33273327
}
33283328
#endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */
3329-
else if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS)
3330-
{
3331-
tls_session_soft_reset(mi->context.c2.tls_multi);
3332-
}
33333329
clear_prefix();
33343330
}
33353331
else
33363332
{
3337-
int msglevel = D_DCO;
3338-
if (dco->dco_message_type == OVPN_CMD_DEL_PEER
3339-
&& dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_USERSPACE)
3340-
{
3341-
/* we receive OVPN_CMD_DEL_PEER message with reason USERSPACE
3342-
* after we kill the peer ourselves. This peer may have already
3343-
* been deleted, so we end up here.
3344-
* In this case, print the following debug message with DCO_DEBUG
3345-
* level only to avoid polluting the standard DCO level with this
3346-
* harmless event.
3347-
*/
3348-
msglevel = D_DCO_DEBUG;
3349-
}
3350-
msg(msglevel, "Received DCO message for unknown peer-id: %d, "
3333+
msg(D_DCO, "Received DCO message for unknown peer-id: %d, "
33513334
"type %d, del_peer_reason %d", peer_id, dco->dco_message_type,
33523335
dco->dco_del_peer_reason);
33533336
}

‎src/openvpn/multi.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@
4343

4444
#define MULTI_PREFIX_MAX_LENGTH 256
4545

46+
47+
void
48+
process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi,
49+
dco_context_t *dco);
50+
4651
/*
4752
* Walk (don't run) through the routing table,
4853
* deleting old entries, and possibly multi_instance

0 commit comments

Comments
 (0)