Patch to apply QoS for DTLS

[Ralph Schmieder]

Hi David. New patch attached, some comments inline.

Thanks,
-ralph

Signed-off-by: Ralph Schmieder <ralph.schmieder at gmail.com>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: oc-qos-v4.patch
Type: application/octet-stream
Size: 7004 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20151017/4051af1b/attachment.obj>
-------------- next part --------------


On Oct 07, 2015, at 11:07 AM GMT+2, David Woodhouse <dwmw2 at infradead.org> wrote:

> On Fri, 2015-08-14 at 18:59 +0200, Ralph Schmieder wrote:
>> Here we go again. Thanks for the comments, hope that I got everything
>> right. For getting the TCLASS I could have used the word instead of
>> the longword, too. But I guess there's no penalty for doing it this
>> way, or is there? And it could use some testing beyond the simple
>> IPv4 in IPv4 use case of mine :)
> 
> Thanks again for working on this, and apologies again for the delay.
> 
> I'm still slightly nervous about the whole concept ? we are
> deliberately leaking information from the inner packet into the outer
> packet. So people will be able to *see* that we're doing VoIP
> traffic.... which in practice they could have inferred quite trivially
> from the packet size and regularity anyway.
> 
> But now I look harder, I see that OpenVPN does already have this
> facility, at least for Legacy IP, with the --passtos option. It's
> disabled by default though, and I wonder if we should do the same. And
> make the option have the same name too?

changed the option to --passtos and given the name it's therefore also disabled by default


> 
> I might ask on the OpenVPN list about passing the values through
> between Legacy IP and IPv6, and propose a patch.
> 
> As for the code... it looks good, in general, with a few minor problems
> remaining:
> 
> The IPV6_TCLASS sockopt requires an 'int', not a 'uint8_t'. I think
> that IP_TOS can also take an 'int' on all platforms (that's what
> OpenVPN uses), so let's just change that in dtls_mainloop().

done, made it an int.


> (Actually, I also wonder if we should just be setting it per-packet by
> using sendmsg(), rather than separately calling setsockopt() each time
> it changes?)


I'm not 100% sure where this change would be applied, probably further down in SSL_write() or gnutls_record_send(). For me, this is good enough from a performance point of view, I'm not sure if this warrants any detailed profiling. I'm running this with multi-megabit video calls and it behaves fine. But that's only my opinion. And since you now have to deliberately enable it I don't see that much of an issue. Is performance your main concern?


> 
> I think your initial value of vpninfo->dtls_tos_current wants to be
> something that's *completely* outside the range of normal values, to
> ensure that it does correctly get set the first time.

When opening a socket, the default is set to 0 anyway. Anyway, I set it to 255 which is way outside the possible values so it should be set for the first packet sent.


> 
> I'm also not sure you're extracting the tclass from the IPv6 header
> correctly:
>      tos = (ntohl(0x0FF00000) & load_be32(this->data)) >> 20;
> I don't
> think the 0x0FF00000 needs to be byte-swapped, does it? You're *always*
> going to get zero with the above version on a little-endian machine?
> 
> The bits you're after are the low 4 bits of the first byte (as the high nybble of tos), and the high 4 bits of the second byte (as low nybble). So I think this would give you the correct result:
>      tos = (load_be16(this->data) >> 4) & 0xff;
> 
> 

thanks... adjusted.


> 
> Finally, I think we need to expose this to the library API with an
> openconnect_set_pass_tos() function.

I added it to openconnect.h and library.c. I also added a few lines to libopenconnect.map.in assuming that this needs a new API version. But totally guessing here. You'll know what to do :)


> 
> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse at intel.com                              Intel Corporation
>