[Openvpn-devel] Check for more data in control channel

  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(+)
  

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