gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding #4189

jfischer-no · 2015-10-29T17:19:39Z

This PR adds NHC UDP decoding and encoding.

@cgundogan @authmillenon @OlegHahm ~~~I need your support to get it pretty.
For first, my mayor problem is that gnrc_pktbuf_realloc_data fails after a few calls.
What would be the alternative?~~~

References:
https://tools.ietf.org/html/rfc6282#section-4.3
https://github.com/torvalds/linux/blob/master/net/6lowpan/nhc_udp.c

miri64 · 2015-10-29T17:24:50Z

@cgundogan @authmillenon @OlegHahm I need your support to get it pretty.
For first, my mayor problem is that gnrc_pktbuf_realloc_data fails after a few calls.
What would be the alternative?

Interesting. Have you found a way to reproduce this isolated (with the unittests e.g.) Would be gread if we could find the reason for that ;).

miri64 · 2015-10-29T17:25:44Z

alternative: (what I did for IPHC) just create a new header for UDP and replace it with the UDP NHC header, when you're done.

miri64 · 2015-10-29T17:27:17Z

sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

@@ -368,7 +369,74 @@ size_t gnrc_sixlowpan_iphc_decode(gnrc_pktsnip_t *ipv6, gnrc_pktsnip_t *pkt, siz

    }

-    /* TODO: add next header decoding */
+    /* next header decoding */


I'd prefer to have this whole chunk of code in a seperate module gnrc_sixlowpan_nhc, so one can turn it of.

jfischer-no · 2015-10-29T17:40:02Z

Interesting. Have you found a way to reproduce this isolated (with the unittests e.g.) Would be gread if we could find the reason for that ;).

Unfortunately not, if I have time I try to investigate.

alternative: (what I did for IPHC) just create a new header for UDP and replace it with the UDP NHC header, when you're done.

That was my first idea, but then I need a pointer to pointer to pkt inside gnrc_sixlowpan_iphc_decode ?

cgundogan · 2015-10-29T19:11:48Z

sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

+
+        /* make place for udp length */
+        if(gnrc_pktbuf_realloc_data(pkt, pkt->size + 2)) {
+            printf("realloc failed\n");


I assume this is just for debugging purpose while implementing. But should be replaced with DEBUG and a proper cleanup pktbuf_free(pkt); return 0;

I am not so sure about pktbuf_free anymore in this context, because it is never used in the above code (@authmillenon?)

jfischer-no · 2015-10-30T18:06:19Z

@authmillenon @cgundogan I have rebuilt it,~~but it still ends in a disaster after some time (after receive and send ca 42 udp packets, 16 ... 11 bytes payload):~~

jfischer-no · 2015-11-02T16:50:14Z

rebased on master, still WIP

decoding is working and can be tested from linux host with something like:

#!/bin/sh

echo "sd-inline" | nc6 -q0 -u fe80::2695:4d4f:5ea9:200a%lowpan0 -p 192 23025
sleep 0.1

echo "s-inline" | nc6 -q0 -u fe80::2695:4d4f:5ea9:200a%lowpan0 -p 23025 61441
sleep 0.1

echo "d-inline" | nc6 -q0 -u fe80::2695:4d4f:5ea9:200a%lowpan0 -p 61442 23025
sleep 0.1

echo "sd-elided" | nc6 -q0 -u fe80::2695:4d4f:5ea9:200a%lowpan0 -p 61617 61617

jfischer-no · 2015-11-04T11:56:34Z

@authmillenon ping

miri64 · 2015-11-04T12:05:57Z

Why is it a pseudo-module? The way you set it up now iphc_nhc_udp_encode() and iphc_nhc_udp_decode() can (if sufficiently prefixed with sixlowpan_iphc_nhc_...) in their own sys/net/gnrc/network_layer/sixlowpan/iphc/nhc/gnrc_sixlowpan_iphc_nhc.c. Also: don't we need the extension header part of RFC 6282 to allow for UDP header compression in case there are extension headers?

miri64 · 2015-11-04T12:06:35Z

Other then that, the code looks great.

jfischer-no · 2015-11-04T15:15:39Z

Why is it a pseudo-module?

From the code structure, I find it more readable if it stays in gnrc_sixlowpan_iphc.c.

Also: don't we need the extension header part of RFC 6282 to allow for UDP header compression in case there are extension headers?

Yes it would be good, I have to read how I can test it.

OlegHahm · 2015-11-06T16:13:13Z

My first initial test found it working. Will test at the ETSI plugtest tomorrow.

OlegHahm · 2015-11-06T16:14:59Z

sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

+    uint8_t *udp_data = udp->data;
+    size_t nhc_len = 0;
+
+    /* TODO: Add support for elided checksum when it is available in the linux kernel. */


We will need also something like IPsec for that.

elided checksum?

Yes. RFC6282 requires authorization of an upper lay in case that e.g. tunneling or a message integrity check is in place by something like IPsec.

Authorization to be allowed eliding the checksum, right?

Yes, exactly.

jfischer-no · 2015-11-06T16:29:15Z

The problem is: how to implement/test it with extension header (https://tools.ietf.org/html/rfc6282#section-4.2). 🌴

OlegHahm · 2015-11-07T00:29:43Z

Luckily this is out of scope for the plugtest. ;)

OlegHahm · 2015-11-08T02:12:05Z

Needs #4235 and jfischer-phytec-iot/RIOT#17 to build for applications without UDP (e.g. gnrc_border_router).

cgundogan · 2015-11-20T16:42:47Z

sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

+
+    /* TODO: Add support for elided checksum when it is available in the linux kernel. */
+
+    /* Compressing UDP ports, follow the same sequence as the linux kernel (nhc_udp module). */


might be nit-picking, but I would be more happy with a reference to the RFC instead of a mentioning a module from the linux kernel.

Can you "fix" this?

It is expressed a little wrong. It means that if first 8 bits of Destination Port are 0xf0 and first 8 bits of Source Port are 0xf0 then dst will be elided. (L504).

Should I remove the comment?

If I look at the code it seems rather that if the first 12 bits of destination and source port are 0xf0b then only four bits are carried inline which is exactly what the RFC says. Or am I mistaken?

no, I mean: if first 8 bits of Destination Port are 0xf0 and first 8 bits of Source Port are 0xf0 there are IMHO two capabilities (RFC leaves free to choose):

dst will be elided

src will be elided

Nothing special, the comment is just confusing.

Okay, whatever. I don't quite agree to your comment, but I agree with your code. ;-) And this is obviously more important. As for the comment: I think it's actually okay to leave as is.

OlegHahm · 2015-12-07T00:22:06Z

Can you rebase, please?

jfischer-no · 2015-12-07T15:19:32Z

@OlegHahm rebased on master, untested

OlegHahm · 2015-12-07T15:20:48Z

Can you also squash my commits?

jfischer-no · 2015-12-07T15:41:07Z

@OlegHahm ok?

OlegHahm · 2015-12-07T15:48:01Z

If you can also replace the SQUASH ME prefix with gnrc 6lowpan iphc, I'm fine.

OlegHahm · 2015-12-07T20:26:50Z

@jfischer-phytec-iot, please address @cgundogan's comment and squash immediately. I tested for RIOT-to-RIOT and RIOT-to-Linux with and without elided ports. Everything works. ACK!

OlegHahm · 2015-12-08T09:25:17Z

Ok, please squash your commits and we're ready to go, I think.

OlegHahm · 2015-12-08T10:26:13Z

Please apply:

diff --git a/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c b/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c
index 6dea595..267f0da 100644
--- a/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c
+++ b/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c
@@ -111,6 +111,7 @@ static inline bool _context_overlaps_iid(gnrc_sixlowpan_ctx_t *ctx,
              (iid->uint8[(ctx->prefix_len / 8) - 8] & byte_mask[ctx->prefix_len % 8])));
 }

+#ifdef MODULE_GNRC_UDP
 inline static size_t iphc_nhc_udp_decode(gnrc_pktsnip_t *pkt, gnrc_pktsnip_t *ipv6, size_t offset)
 {
     uint8_t *payload = pkt->data;
@@ -181,6 +182,7 @@ inline static size_t iphc_nhc_udp_decode(gnrc_pktsnip_t *pkt, gnrc_pktsnip_t *ip

     return offset;
 }
+#endif

 size_t gnrc_sixlowpan_iphc_decode(gnrc_pktsnip_t *ipv6, gnrc_pktsnip_t *pkt, size_t datagram_size,
                                   size_t offset)

and squash immediately.

jfischer-no · 2015-12-08T12:46:47Z

@OlegHahm Travis is right, it needs to be fixed, ~~are you on it or should I?~~ #4437

OlegHahm · 2015-12-08T13:45:25Z

It's probably easiest if you just do it. I'm without access to a real computer currently anyway .

On 8 December 2015 13:46:49 CET, Johann Fischer notifications@github.com wrote:

@OlegHahm Travis is right, it needs to be fixed, are you on it or
should I?

Reply to this email directly or view it on GitHub:
#4189 (comment)

Join the RIOT
http://www.riot-os.org

This patch adds UDP Header Compression and Decompression as specified in https://tools.ietf.org/html/rfc6282#section-4.3 UDP NHC module will be included only if UDP is used.

gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding

miri64 · 2015-12-15T17:08:06Z

sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

+    ipv6_hdr->nh = PROTNUM_UDP;
+    ipv6_hdr->len = udp_hdr->length;
+
+    ipv6->next = udp;


This is the cause for #4397: Decoding happens on receive, this means the udp snip must come before the IPv6 snip. But since the IPv6 snip is only inserted after decoding, this is not easy to fix. Will try anyway :-)

jfischer-no added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 29, 2015

jfischer-no assigned miri64 Oct 29, 2015

miri64 reviewed Oct 29, 2015
View reviewed changes

cgundogan reviewed Oct 29, 2015
View reviewed changes

This was referenced Nov 2, 2015

sys/net/gnrc/conn/gnrc_conn.c: release pkt in gnrc_conn_recvfrom #4207

Merged

sys/net/gnrc/conn/gnrc_conn.c: release pkt in gnrc_conn_recvfrom #4209

Merged

jfischer-no added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 2, 2015

jfischer-no changed the title ~~gnrc_sixlowpan_iphc.c: add nhc udp decoding~~ gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding Nov 4, 2015

jfischer-no removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 4, 2015

OlegHahm reviewed Nov 6, 2015
View reviewed changes

cgundogan reviewed Nov 20, 2015
View reviewed changes

This was referenced Dec 1, 2015

Initial TFTP support #3911

Merged

TFTP fixes DipSwitch/RIOT#4

Closed

TFTP fixes DipSwitch/RIOT#5

Merged

OlegHahm added this to the Release 2015.12 milestone Dec 7, 2015

miri64 mentioned this pull request Dec 7, 2015

gnrc_sixlowpan: Header order or UDP and IPv6 swapped #4397

Closed

OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 8, 2015

jfischer-no added 3 commits December 8, 2015 14:50

gnrc_sixlowpan_iphc.c: add nhc udp decoding and encoding

a85d50b

This patch adds UDP Header Compression and Decompression as specified in https://tools.ietf.org/html/rfc6282#section-4.3 UDP NHC module will be included only if UDP is used.

gnrc_udp.c adapt for nhc udp encoding and decoding

c6ab57f

makefiles: add gnrc_sixlowpan_iphc_nhc module

994dd19

OlegHahm added a commit that referenced this pull request Dec 8, 2015

Merge pull request #4189 from jfischer-phytec-iot/wip@nhc-udp

693d438

gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding

OlegHahm merged commit 693d438 into RIOT-OS:master Dec 8, 2015

jfischer-no deleted the wip@nhc-udp branch December 8, 2015 15:50

miri64 mentioned this pull request Dec 11, 2015

IPHC/NHC broken #4462

Closed

miri64 reviewed Dec 15, 2015
View reviewed changes


		/* TODO: Add support for elided checksum when it is available in the linux kernel. */

		/* Compressing UDP ports, follow the same sequence as the linux kernel (nhc_udp module). */

gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding #4189

gnrc_sixlowpan_iphc.c: add nhc udp encoding and decoding #4189

Conversation

jfischer-no commented Oct 29, 2015

miri64 commented Oct 29, 2015

miri64 commented Oct 29, 2015

Choose a reason for hiding this comment

jfischer-no commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfischer-no commented Oct 30, 2015

jfischer-no commented Nov 2, 2015

jfischer-no commented Nov 4, 2015

miri64 commented Nov 4, 2015

miri64 commented Nov 4, 2015

jfischer-no commented Nov 4, 2015

OlegHahm commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfischer-no commented Nov 6, 2015

OlegHahm commented Nov 7, 2015

OlegHahm commented Nov 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegHahm commented Dec 7, 2015

jfischer-no commented Dec 7, 2015

OlegHahm commented Dec 7, 2015

jfischer-no commented Dec 7, 2015

OlegHahm commented Dec 7, 2015

OlegHahm commented Dec 7, 2015

OlegHahm commented Dec 8, 2015

OlegHahm commented Dec 8, 2015

jfischer-no commented Dec 8, 2015

OlegHahm commented Dec 8, 2015

Choose a reason for hiding this comment