| Message ID | 1515067670-13094-1-git-send-email-steffan.karger@fox-it.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Check for more data in control channel | expand |
On 04/01/18 13:07, Steffan Karger wrote: > If control channel packets arrive quickly after each other, or out of > order, there might be more data available than we can read in one > tls_process() call. If that happened, and no further control channel > packet arrived (e.g. because the last two packets arrived out-of-order), > we would wait for 16 second ("coarse timer") before we would read the > remaining data. To avoid that, always schedule ourself again if there > was control channel data, to check whether more data is available. > > For mbedtls, we could implement a slightly more elegant "is there more > data?" function, instead of blindly rescheduling. But I can't find a way > to implement that for OpenSSL, and the current solution is very simple and > still has quite low overhead. I haven't looked at the code paths yet (except of the patch itself) ... but how will this affect a server config with a bit of load? Like some hundred connected clients or more? Will these other clients notice that a client gets rescheduled instantly? And as well, what if more clients trigger this behaviour approximately in the same time window?
Hi David, On 05-01-18 20:48, David Sommerseth wrote: > On 04/01/18 13:07, Steffan Karger wrote: >> If control channel packets arrive quickly after each other, or out of >> order, there might be more data available than we can read in one >> tls_process() call. If that happened, and no further control channel >> packet arrived (e.g. because the last two packets arrived out-of-order), >> we would wait for 16 second ("coarse timer") before we would read the >> remaining data. To avoid that, always schedule ourself again if there >> was control channel data, to check whether more data is available. >> >> For mbedtls, we could implement a slightly more elegant "is there more >> data?" function, instead of blindly rescheduling. But I can't find a way >> to implement that for OpenSSL, and the current solution is very simple and >> still has quite low overhead. > > I haven't looked at the code paths yet (except of the patch itself) ... but > how will this affect a server config with a bit of load? Like some hundred > connected clients or more? Will these other clients notice that a client gets > rescheduled instantly? And as well, what if more clients trigger this > behaviour approximately in the same time window? Good question. One extra tls_process() loop for each connecting client should have barely any effect. Based on the code, I would expect it to be less processing than a single data channel packet. For more certainty, I ran some tests. This is the average time for setting up 50 connections simultaneously without bandwidth limits or packet loss (om my 5-year old dual core laptop), and waiting for all 50 to finish (average over 20 runs): Without the patch: Mean: 11.16 Stdev: 0.35 With the patch: Mean: 11.14 Stdev: 0.36 Same test, but now with an iperf maxing out the data channel at the same time (laptop turns into a blowdryer...): Without the patch: Mean: 11.17 Stdev: 0.68 With the patch: Mean: 11.33 Stdev: 0.55 (iperf reported for both a little over 400 Mbit/s) I think these numbers are a strong indication that my 'barely any effect' estimate is correct. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 So I've glared a bit on the code, and it makes sense to me (while not claiming I fully understand the full timer logic and scheduling). Smoke tested patch on RHEL7 (client) and Fedora 27 (server) and tested server code using the openvpn3-linux client as well. Everything seems to work as expected, including 'make check'. And since James didn't have any objections to this approach, I consider this safe to go. Acked-by: David Sommerseth <davids@openvpn.net> Your patch has been applied to the following branches commit b00d56e1b0cf4d71dc4944ef14ea7eca2fc8c519 (master) commit 8f15fa94dd9d7c4ce2dfe3378bd6311e2ad121ba (release/2.4) Author: Steffan Karger Date: Thu Jan 4 13:07:50 2018 +0100 Check for more data in control channel Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: David Sommerseth <davids@openvpn.net> Message-Id: <1515067670-13094-1-git-send-email-steffan.karger@fox-it.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16151.html Signed-off-by: David Sommerseth <davids@openvpn.net> - -- kind regards, David Sommerseth -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBCgAGBQJaoGN5AAoJEIbPlEyWcf3ywZwQAJk0JkYcEw0PX0QbWGDsaY7F QqRMtp4kH9Agsx1/P2Sr5B8H08XBaWDrf+2u/qcZxefB5J7b6CCAR8ueqz+vBtOv PmUvHGrGKOZiqF2pQdqcZ+3pvljLVgXcw0uKOu4MM7TBeVhIN93F902xJE3r4cju G3K3Nv4CkgokNI2I/XBrZWJxeBSUn7gwuAwpIEcvo9LVMoaA1H24xdpIj39W7GdO qnouQUyQSo7UIufrkfqc06+inGGlQzSm31nVM3nGVMLOSnPi6Mjpo6GVhfLhEZMT R/IHnSL1P0/b1GXXe1OFmTw2NSIj+L2dixxFEhaaaUcfWzHn+EfnZiQNd6b+AEMV aqhJZfoQncIKLS3P4by27PSGEs3L416HGDUUGnFHtQxLw1YCUv8E1QvAznrGi2Ek m8lNntoW70y8jrHMnnkdrWI+/Ert0G0ZPDgl0O2ejnje7Y6FLgeU7HDu6dIC/2xs Xe55bdZi5rxVtgjh/jsrIZpi+yYorBKJSGKcZvappwjCGwzjY42biSOlo3dMrvFJ ASHYrzH47lRz+Wxl8Eixy43hcVqYYuebEkbzuLcMOF5LlNjX1biE8CrCgbNkL59t 9ClW9uglx7yfUP+BwgIzo/sD4s2IcAdE3dd3+v2QBvownuC6su+PQzo7GXfeeMlJ Xg514b81EsUdEeT4r5gY =C8xP -----END PGP SIGNATURE----- ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 7b42845..15a37a3 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2935,6 +2935,9 @@ tls_process(struct tls_multi *multi, { state_change = true; dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext"); + + /* More data may be available, wake up again asap to check. */ + *wakeup = 0; } }
If control channel packets arrive quickly after each other, or out of order, there might be more data available than we can read in one tls_process() call. If that happened, and no further control channel packet arrived (e.g. because the last two packets arrived out-of-order), we would wait for 16 second ("coarse timer") before we would read the remaining data. To avoid that, always schedule ourself again if there was control channel data, to check whether more data is available. For mbedtls, we could implement a slightly more elegant "is there more data?" function, instead of blindly rescheduling. But I can't find a way to implement that for OpenSSL, and the current solution is very simple and still has quite low overhead. Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> --- src/openvpn/ssl.c | 3 +++ 1 file changed, 3 insertions(+)