tinydtls: add sock_async support for sock_dtls

[miri64]

Contribution description

A proposal for sock_async with sock_dtls. This version is not really compileable due to some header foobar...

Testing procedure

This is just meant of the PoC (that sadly doesn't compile :-/) so no tests for now. We could update examples/dtls-sock if we want to though.

Issues/PRs references

Requires #12921 (merged)

[miri64]

Added #12921 as a dependency for the cyclic header dependency problem.

[miri64]

Rebased to current master. No longer dependent on other PRs.

[MichelRottleuthner]

@miri64 what did you use for testing this? I tried adapting examples/dtls-sock, but either something isn't working as it should or I am missing something.
I expected something like adding callbacks (i.e. sock_dtls_cb_t) and moving the read operations to the callback (or dispatching it from there) should be enough to make use of async sock.
But in dtls-server.c this lead to the callback for SOCK_ASYNC_MSG_RECV never actually being called when sending a message from the client. Is there something else I need to do?

[miri64]

I did the port before sock_async was merged. So there was no testing possible yet. Will have a look, now that this happened.

[miri64]

1. I rebased to current master and fixed a compile time error
2. I tried to use this patch, however when trying to follow the README of this example I get the following error on native:

> dtlsc fe80::bf:bbff:fe52:6438 "DATA to send"
dtlsc fe80::bf:bbff:fe52:6438 "DATA to send"
Error creating session: -110

(110 is ETIMEDOUT on my system)

[miri64]

I have the same problem if I revert the patch. I try on master, but given that the code should be unchanged without that patch, I expect it to not work there as well.

[miri64]

Same problem on master. Here's what I get in wireshark:

[MichelRottleuthner]

Thanks for testing that. The patch boils down to what I tried before.

I tried to use this patch, however when trying to follow the README of this example I get the following error on native:
dtlsc fe80::bf:bbff:fe52:6438 "DATA to send"
dtlsc fe80::bf:bbff:fe52:6438 "DATA to send"
Error creating session: -110

With your patch applied I notice an interesting difference depending on when I execute the commands.
When executing dtlss start, then waiting for ~20 seconds and then running dtlsc I get the same as you.
But without waiting I get this:

native0> dtlss start
dtlss start

*no waiting here*

native1> dtlsc fe80::e43a:a5ff:feba:b8cc "test data"                     
dtlsc fe80::e43a:a5ff:feba:b8cc "test data"
Session became ready
DTLS message was sent
Sent DTLS message
Terminating

native0> make: *** [/home/michel/devel/riot/examples/dtls-sock/../../Makefile.include:685: term] Segmentation fault (core dumped)
make: Leaving directory '/home/michel/devel/riot/examples/dtls-sock'

Here is a backtrace of the crash

#0  0x565d4f30 in _dtls_server_stack ()
#1  0x565dd030 in main_stack ()
#2  0x565dd030 in main_stack ()
#3  0x5659d181 in _event (ctx=0x565edbe0 <dtlscontext_storage_data>, 
    session=0x565edf84 <peer_storage_data+4>, level=(unknown: 0), code=478)
    at /home/michel/devel/riot/pkg/tinydtls/contrib/sock_dtls.c:158
#4  0x565b23c7 in dtls_handle_message (ctx=0x565edbe0 <dtlscontext_storage_data>, 
    session=0x565d4e78 <_dtls_server_stack+35512>, 
    msg=0x565d4ffc <_dtls_server_stack+35900> "\026\376", <incomplete sequence \375>, msglen=53)
    at /home/michel/devel/riot/examples/dtls-sock/bin/pkg/native/tinydtls/dtls.c:3986
#5  0x5659daa2 in sock_dtls_recv (sock=0x565d4ed0 <_dtls_server_stack+35600>, 
    remote=0x565d4e60 <_dtls_server_stack+35488>, data=0x565d4ffc <_dtls_server_stack+35900>, 
    max_len=512, timeout=8578042)
    at /home/michel/devel/riot/pkg/tinydtls/contrib/sock_dtls.c:459
#6  0x5658d4cd in dtls_server_wrapper (arg=0x0)
    at /home/michel/devel/riot/examples/dtls-sock/dtls-server.c:115
#7  0xf7c396cf in makecontext () from /usr/lib32/libc.so.6
#8  0x00000000 in ?? ()

Same problem on master. Here's what I get in wireshark:

On master I get the timeout also only when waiting ~20 secods between starting the server and starting the client. But it works correctly without waiting..

[miri64]

I can confirm that there is no timeout if the client sends immediately after server creation, and I can reproduce the crash.

[miri64]

Fixed the course of the crash. Now to investigate why SOCK_ASYNC_MSG_RECV is never fired.

[MichelRottleuthner]

(...) Now to investigate why SOCK_ASYNC_MSG_RECV is never fired.

Maybe this helps finding the right direction: when I tested this before the callback mechanism seemed working okay if I called a "normal" sock_dtls_recv beforehand (i.e. before expecting a callback) but I didn't dig deeper from there.

[miri64]

Yes, that helped. The _read() function is basically left here

RIOT/pkg/tinydtls/contrib/sock_dtls.c

Lines 92 to 95 in 2f9d1b3

	if (sock->buflen < len && sock->buf) {
	DEBUG("sock_dtls: not enough place on buffer for decrypted message\n");
	msg.content.value = -ENOBUFS;
	}

As only with sock_dtls_recv() being called sock->buf and sock->buflen are initialized.

RIOT/pkg/tinydtls/contrib/sock_dtls.c

Lines 438 to 440 in 2f9d1b3

	/* save location to result buffer */
	sock->buf = data;
	sock->buflen = max_len;

So we need to get rid of that state somehow to allow for asynchronous receive :-/.

[miri64]

Wait... do I understand this correctly: _read() is called from within dtls_handle_message() which is called from within sock_dtls_recv() and _read() is filling the sock's mbox that is then emptied within sock_dtls_recv() again? Am I missing something? I think the indirection via the mbox is not necessary.

[miri64]

Or is dtls_handle_message() blocking?

[MichelRottleuthner]

Not sure on the actual implementation but I think this has to do with the fact that dtls_handle_message may require multiple calls before returning application data and it seems it's return value doesn't indicate what kind of event happened.

Or is dtls_handle_message() blocking?

AIUI it only handles the data you hand to it - so what should it wait for?
I bet @pokgak can answer this better than me.

[miri64]

I added the tests here after all and not in a separate PR, as I pointed out, there are some issues (the one previously mentioned however, is fixed now):

1. After the first message from the client, sending another message is not possible
2. see inline comment below.

[pokgak]

Okay, I found the problem. When sending the second message, we actually already have a msg in mbox for the CLOSE_NOTIFY alert from the previous session. So that is why we don't see the DTLS_EVENT_CONNECTED msg when doing mbox_try_get in sock_dtls_recv for the second message. The following suggestion should fix this. I totally forgot to consider this in the earlier versions 🤦

[miri64]

Squashed, rebased and adapted to current master. Test now works without any problems :-).

[miri64]

@benpicco @MichelRottleuthner can one of you have another look?

[benpicco]

Looks good to me, I can confirm the test is working.

Travis spotted two minor issues, you can squash the fixes in directly.

[miri64]

Squashed.

[miri64]

And another squash.

[benpicco]

No objections from my side - if there are still bugs, we will find them as we go about implementing the layer that goes on top :)

[miri64]

if there are still bugs, we will find them as we go about implementing the layer that goes on top :)

And other layers below when we e.g. finally implement sock_dtls for wolfssl :-).

[pokgak]

Thanks @miri64 for working on this and @benpicco for the review :)

[benpicco]

I'm currently trying to implement sock_dtls_recv_buf_aux() so I got back to this function.
How is this supposed to work? The way this is implemented (the while loop) this will just flush (and discard) pending data on the socket.

On tangent: Is there even such a thing as 'chunked datagram payload'?
As far as I can see it is implemented in no network stack, it's always

• first call returns pointer to the buffer
• second call frees the buffer

The only way I could see we could get chunked datagram payload is if we didn't do IP fragment reassembly in the stack but pushed it to the user to handle.

That seems not like something anyone would want. And user would need to maintain a reassembly buffer, nullifying the zero-copy advantage of sock_udp_recv_buf().

Is there any real world use for this API that we can't do a single buffer lease -> return?

[miri64]

On tangent: Is there even such a thing as 'chunked datagram payload'?

Yes, lwIP is returning ~500B---I don't remember the exact number and don't want to stall answering this question by looking it up---non-continuous chunks of data for larger UDP packets. Not sure if this is still the case, after the packet went to TinyDTLS or if other DTLS implementations might still opt to chunk the payload (e.g. by data record in the actual DTLS packet).

How is this supposed to work?

Due to the restrictions of TinyDTLS that you I believe fixed a proper implementation was not possible so far. The idea is to call this function repeatedly, until there are no chunks left (signified by return value 0). I think the lwip_sock_udp implementation provides a good example of how this is supposed to look like.