Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1751?usp=email
to look at the new patch set (#22).
Change subject: oob: Probe every resolved address of a remote
......................................................................
oob: Probe every resolved address of a remote
A remote can resolve to several A/AAAA records; --server-probe
previously sent a SERVER_PROBE only to the first one, so a remote whose
first address was unreachable looked unresponsive even if another
address answered, and the connection could later try an address that was
never probed.
Probe every resolved address of each remote in parallel (per-remote
storage is sized to the resolved address count, no fixed cap), accept a
reply from any of them (first reply for a remote wins), and resend to
all of a remote's addresses while it is unanswered. The per-address send
moves into oob_probe_send_target(), which keeps the
one-probe-per-address rule across the addresses of all entries.
Change-Id: I00208610e0ce4f3ed9a87233e68ece1b3b8a768a
Signed-off-by: Lev Stipakov <[email protected]>
---
M doc/man-sections/client-options.rst
M src/openvpn/oob.c
M src/openvpn/oob.h
M src/openvpn/oob_client.c
M tests/unit_tests/openvpn/test_oob.c
5 files changed, 178 insertions(+), 83 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/51/1751/22
diff --git a/doc/man-sections/client-options.rst
b/doc/man-sections/client-options.rst
index 8b6bb2f..84a9f51 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -604,7 +604,7 @@
server-probe
server-probe max-latency-diff
- A small probe message is sent to the first resolved address of every UDP
+ A small probe message is sent to every resolved address of every UDP
remote, and each answering server replies with its advertised priority
and weight. Remotes are then reordered following DNS SRV (RFC 2782)
semantics: servers that answered are tried before those that did not,
diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index 20b3a4f..7855b71 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -154,9 +154,16 @@
{
for (int i = start; i < n; i++)
{
- if (targets[i].sent && !results[i].responded && addr_port_match(from,
&targets[i].dest))
+ if (!targets[i].sent || results[i].responded)
{
- return i;
+ continue;
+ }
+ for (int k = 0; k < targets[i].n_dests; k++)
+ {
+ if (addr_port_match(from, &targets[i].dests[k]))
+ {
+ return i;
+ }
}
}
return -1;
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index ebdcd3f..e76745d 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -190,11 +190,12 @@
struct oob_probe_reply reply; /* the values the server advertised */
};
-/* Where the client probed one connection entry. */
+/* Where the client probed one connection entry: its resolved addresses. */
struct oob_probe_target
{
- struct openvpn_sockaddr dest;
- socklen_t destlen;
+ struct openvpn_sockaddr *dests;
+ socklen_t *destlens;
+ int n_dests;
bool sent;
struct timeval sent_at; /* when the first probe went out, for the RTT */
};
diff --git a/src/openvpn/oob_client.c b/src/openvpn/oob_client.c
index c3e2e2b..871b5d9 100644
--- a/src/openvpn/oob_client.c
+++ b/src/openvpn/oob_client.c
@@ -321,6 +321,83 @@
}
}
+/* Does any address of t have a probe socket of its family? */
+static bool
+oob_probe_target_reachable(const struct probe_ctx *pc, const struct
oob_probe_target *t)
+{
+ for (int k = 0; k < t->n_dests; k++)
+ {
+ if (pc->sd[probe_af_index(t->dests[k].addr.sa.sa_family)] !=
SOCKET_UNDEFINED)
+ {
+ return true;
+ }
+ }
+ return false;
+}
+
+/* Send the probe to every resolved address of t, each on the socket matching
+ * its address family, recording when the first one went out so the reply's RTT
+ * is measured from there. An address an earlier entry already probed in this
+ * round is not probed again; that probe's reply covers this entry too. Returns
+ * true if at least one address was sent to or is already covered. */
+static bool
+oob_probe_send_target(struct probe_ctx *pc, const struct buffer *probe, struct
oob_probe_target *t)
+{
+ bool any_sent = false;
+ /* The RTT runs from the first send. A reply carries nothing that ties it
to
+ * a send round, so measuring from a resend would report a server slower
+ * than one round as fast. */
+ bool stamped = t->sent;
+ for (int k = 0; k < t->n_dests; k++)
+ {
+ const int af = probe_af_index(t->dests[k].addr.sa.sa_family);
+ socket_descriptor_t sd = pc->sd[af];
+ if (sd == SOCKET_UNDEFINED)
+ {
+ continue;
+ }
+#ifdef TARGET_ANDROID
+ /* Keep the probe out of an active tunnel, as the connection socket is.
+ * protect_fd_nonlocal() declines a local peer, so only a non-local one
+ * actually protects the socket and may latch the flag. */
+ if (!pc->fd_protected[af] && !addr_local(&t->dests[k].addr.sa))
+ {
+ protect_fd_nonlocal(sd, &t->dests[k].addr.sa);
+ pc->fd_protected[af] = true;
+ }
+#endif
+ const int seen = oob_addr_list_find(pc->probed, pc->n_probed,
&t->dests[k]);
+ if (seen >= 0)
+ {
+ /* Already probed this round by an earlier entry: share that send
+ * time, so both entries report the same RTT for the one reply. */
+ if (!stamped)
+ {
+ t->sent_at = pc->probed_at[seen];
+ stamped = true;
+ }
+ any_sent = true;
+ continue;
+ }
+ struct timeval sent_at;
+ openvpn_gettimeofday(&sent_at, NULL);
+ if (sendto(sd, (const char *)CBPTR(probe), (int)BLEN(probe), 0,
+ (const struct sockaddr *)&t->dests[k], t->destlens[k])
+ >= 0)
+ {
+ if (!stamped)
+ {
+ t->sent_at = sent_at;
+ stamped = true;
+ }
+ pc->probed_at[pc->n_probed] = sent_at;
+ pc->probed[pc->n_probed++] = t->dests[k];
+ any_sent = true;
+ }
+ }
+ return any_sent;
+}
+
/* Parse one received datagram as a PROBE_REPLY and, if valid and matching one
* of the probes we sent, record the reply in results. */
static void
@@ -363,9 +440,10 @@
return;
}
- /* Credit the reply to every still-unanswered remote probed at its source
- * address: several entries can resolve to the same address, and each takes
- * the first reply for it. */
+ /* Credit the reply to every still-unanswered remote that was probed at its
+ * source address: several entries can resolve to the same address, and
each
+ * takes the first reply for it. (A remote with several addresses may
answer
+ * from more than one; the first one wins.) */
struct timeval rcv;
openvpn_gettimeofday(&rcv, NULL);
int i = oob_probe_next_target_at(from, targets, results, n, 0);
@@ -474,27 +552,9 @@
pc->n_probed = 0;
for (int i = 0; i < n; i++)
{
- if (!targets[i].sent || results[i].responded)
+ if (targets[i].sent && !results[i].responded)
{
- continue;
- }
- if (oob_addr_list_find(pc->probed, pc->n_probed, &targets[i].dest) >=
0)
- {
- continue;
- }
- socket_descriptor_t sd =
pc->sd[probe_af_index(targets[i].dest.addr.sa.sa_family)];
- if (sd == SOCKET_UNDEFINED)
- {
- continue;
- }
- struct timeval sent_at;
- openvpn_gettimeofday(&sent_at, NULL);
- if (sendto(sd, (const char *)CBPTR(probe), (int)BLEN(probe), 0,
- (const struct sockaddr *)&targets[i].dest,
targets[i].destlen)
- >= 0)
- {
- pc->probed_at[pc->n_probed] = sent_at;
- pc->probed[pc->n_probed++] = targets[i].dest;
+ oob_probe_send_target(pc, probe, &targets[i]);
}
}
}
@@ -700,11 +760,23 @@
continue;
}
- /* Store the first resolved address natively (no IPv4-mapping); it is
- * later probed on its family's socket. */
- memcpy(&targets[i].dest, ai->ai_addr, ai->ai_addrlen);
- targets[i].destlen = (socklen_t)ai->ai_addrlen;
- need_af[probe_af_index(ai->ai_family)] = true;
+ /* Store every resolved address natively (no IPv4-mapping); each is
later
+ * probed on its family's socket. */
+ struct oob_probe_target *t = &targets[i];
+ int n_addr = 0;
+ for (const struct addrinfo *a = ai; a; a = a->ai_next)
+ {
+ n_addr++;
+ }
+ t->dests = gc_malloc(sizeof(*t->dests) * n_addr, true, &gc);
+ t->destlens = gc_malloc(sizeof(*t->destlens) * n_addr, true, &gc);
+ for (const struct addrinfo *a = ai; a; a = a->ai_next)
+ {
+ memcpy(&t->dests[t->n_dests], a->ai_addr, a->ai_addrlen);
+ t->destlens[t->n_dests] = (socklen_t)a->ai_addrlen;
+ t->n_dests++;
+ need_af[probe_af_index(a->ai_family)] = true;
+ }
freeaddrinfo(ai);
}
@@ -721,58 +793,32 @@
return;
}
- /* Send a probe to each resolved remote, on the socket of its address
family.
- * An address several entries resolve to is probed once; its reply is
- * credited to each of them. */
- pc.probed = gc_malloc(sizeof(*pc.probed) * l->len, false, &gc);
- pc.probed_at = gc_malloc(sizeof(*pc.probed_at) * l->len, false, &gc);
+ /* Send a probe to each resolved remote, on the socket of each address. */
+ int n_addrs = 0;
+ for (int i = 0; i < l->len; i++)
+ {
+ n_addrs += targets[i].n_dests;
+ }
+ pc.probed = gc_malloc(sizeof(*pc.probed) * n_addrs, false, &gc);
+ pc.probed_at = gc_malloc(sizeof(*pc.probed_at) * n_addrs, false, &gc);
pc.n_probed = 0;
int sent_count = 0;
for (int i = 0; i < l->len; i++)
{
const struct connection_entry *ce = l->array[i];
- if (!targets[i].destlen)
+ struct oob_probe_target *t = &targets[i];
+ if (t->n_dests == 0 || !oob_probe_target_reachable(&pc, t))
{
+ continue; /* nothing resolved, or its family is not probed */
+ }
+
+ if (!oob_probe_send_target(&pc, &probe, t))
+ {
+ msg(D_LOW, "server-probe: %s:%s: probe send failed", ce->remote,
ce->remote_port);
continue;
}
- const int af = probe_af_index(targets[i].dest.addr.sa.sa_family);
- socket_descriptor_t sd = pc.sd[af];
- if (sd == SOCKET_UNDEFINED)
- {
- continue; /* family not probed, see oob_probe_sockets_open() */
- }
-#ifdef TARGET_ANDROID
- /* Keep the probe out of an active tunnel, as the connection socket is.
- * protect_fd_nonlocal() declines a local peer, so only a non-local one
- * actually protects the socket and may latch the flag. */
- if (!pc.fd_protected[af] && !addr_local(&targets[i].dest.addr.sa))
- {
- protect_fd_nonlocal(sd, &targets[i].dest.addr.sa);
- pc.fd_protected[af] = true;
- }
-#endif
- const int seen = oob_addr_list_find(pc.probed, pc.n_probed,
&targets[i].dest);
- if (seen >= 0)
- {
- /* Already probed this round by an earlier entry: reuse that send
- * time, so both entries report the same RTT for the one reply. */
- targets[i].sent_at = pc.probed_at[seen];
- }
- else
- {
- openvpn_gettimeofday(&targets[i].sent_at, NULL);
- if (sendto(sd, (const char *)BPTR(&probe), (int)BLEN(&probe), 0,
- (struct sockaddr *)&targets[i].dest, targets[i].destlen)
- < 0)
- {
- msg(D_LOW, "server-probe: %s:%s: probe send failed",
ce->remote, ce->remote_port);
- continue;
- }
- pc.probed_at[pc.n_probed] = targets[i].sent_at;
- pc.probed[pc.n_probed++] = targets[i].dest;
- }
- targets[i].sent = true;
+ t->sent = true;
sent_count++;
}
diff --git a/tests/unit_tests/openvpn/test_oob.c
b/tests/unit_tests/openvpn/test_oob.c
index 6090d6e..7092f94 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -518,10 +518,12 @@
return sa;
}
+/* One probed address per entry. */
static struct oob_probe_target
-probe_target(struct openvpn_sockaddr dest, bool sent)
+probe_target(struct openvpn_sockaddr *dest, bool sent)
{
- return (struct oob_probe_target){ .dest = dest, .destlen =
sizeof(dest.addr.in4), .sent = sent };
+ static socklen_t len = sizeof(struct sockaddr_in);
+ return (struct oob_probe_target){ .dests = dest, .destlens = &len,
.n_dests = 1, .sent = sent };
}
/* Two entries resolving to the same address are both credited by one reply,
@@ -530,7 +532,7 @@
test_probe_reply_credits_every_entry_at_address(void **state)
{
struct openvpn_sockaddr a = v4_addr(0x7f000001, 1194);
- struct oob_probe_target targets[2] = { probe_target(a, true),
probe_target(a, true) };
+ struct oob_probe_target targets[2] = { probe_target(&a, true),
probe_target(&a, true) };
struct oob_probe_result results[2] = { 0 };
int i = oob_probe_next_target_at(&a, targets, results, 2, 0);
@@ -554,9 +556,9 @@
struct openvpn_sockaddr b = v4_addr(0x7f000002, 1194);
struct openvpn_sockaddr a_other_port = v4_addr(0x7f000001, 1195);
struct oob_probe_target targets[3] = {
- probe_target(a, true),
- probe_target(b, true),
- probe_target(a, false), /* resolved but never sent */
+ probe_target(&a, true),
+ probe_target(&b, true),
+ probe_target(&a, false), /* resolved but never sent */
};
struct oob_probe_result results[3] = { 0 };
@@ -567,6 +569,43 @@
assert_int_equal(oob_probe_next_target_at(&a_other_port, targets, results,
3, 0), -1);
}
+/* A remote with several addresses is credited whichever of them answers. */
+static void
+test_probe_reply_matches_any_address_of_entry(void **state)
+{
+ struct openvpn_sockaddr addrs[2] = { v4_addr(0x7f000001, 1194),
v4_addr(0x7f000002, 1194) };
+ socklen_t lens[2] = { sizeof(struct sockaddr_in), sizeof(struct
sockaddr_in) };
+ struct oob_probe_target targets[1] = {
+ { .dests = addrs, .destlens = lens, .n_dests = 2, .sent = true },
+ };
+ struct oob_probe_result results[1] = { 0 };
+
+ assert_int_equal(oob_probe_next_target_at(&addrs[1], targets, results, 1,
0), 0);
+ assert_int_equal(oob_probe_next_target_at(&addrs[0], targets, results, 1,
0), 0);
+}
+
+/* IPv6 addresses match on address and port too. */
+static void
+test_probe_addresses_ipv6(void **state)
+{
+ struct openvpn_sockaddr a = { 0 }, b = { 0 };
+ a.addr.in6.sin6_family = AF_INET6;
+ a.addr.in6.sin6_addr.s6_addr[15] = 1; /* ::1 */
+ a.addr.in6.sin6_port = htons(1194);
+ b = a;
+ b.addr.in6.sin6_port = htons(1195);
+ socklen_t len = sizeof(struct sockaddr_in6);
+ struct oob_probe_target targets[1] = {
+ { .dests = &a, .destlens = &len, .n_dests = 1, .sent = true },
+ };
+ struct oob_probe_result results[1] = { 0 };
+
+ assert_int_equal(oob_probe_next_target_at(&a, targets, results, 1, 0), 0);
+ assert_int_equal(oob_probe_next_target_at(&b, targets, results, 1, 0), -1);
+ assert_int_equal(oob_addr_list_find(&a, 1, &a), 0);
+ assert_int_equal(oob_addr_list_find(&a, 1, &b), -1);
+}
+
/* The per-round list of probed addresses compares address and port. */
static void
test_addr_list_find(void **state)
@@ -972,6 +1011,8 @@
cmocka_unit_test(test_server_probe_accept_no_parameter),
cmocka_unit_test(test_probe_reply_credits_every_entry_at_address),
cmocka_unit_test(test_probe_reply_matches_only_its_address),
+ cmocka_unit_test(test_probe_reply_matches_any_address_of_entry),
+ cmocka_unit_test(test_probe_addresses_ipv6),
cmocka_unit_test(test_addr_list_find),
cmocka_unit_test(test_client_reply_read_finds_reply),
cmocka_unit_test(test_client_reply_read_skips_unknown),
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1751?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I00208610e0ce4f3ed9a87233e68ece1b3b8a768a
Gerrit-Change-Number: 1751
Gerrit-PatchSet: 22
Gerrit-Owner: stipa <[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
