gnrc_sixlowpan: Fix IPHC/NHC packet order problem #4485

miri64 · 2015-12-15T19:07:04Z

Fixes #4397 (and maybe #4462). Could not test because for some reason I can't send anything right now (even with txtsnd).

Here is the general idea of this fix: the ipv6 parameter of gnrc_sixlowpan_iphc_decode() is replaced with the out-parameter dec_hdr. While gnrc_sixlowpan_iphc_decode() handles the dereferenced pointer the same as it used to handle ipv6 iphc_nhc_udp_decode() sets it to the UDP header and appends the IPv6 header (the old value of dec_hdr) to the UDP header. This way the correct header order should be preserved.

Depends on ~~#4507~~ (merged).

OlegHahm · 2015-12-15T20:24:44Z

Most likely you cannot send because of #4482.

miri64 · 2015-12-15T22:33:43Z

I wait in anticipation (can't test for now anyway).

jfischer-no · 2015-12-15T23:13:25Z

looks good, will test it during the day.

jfischer-no · 2015-12-17T12:58:21Z

https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L729
will drop the packet:
gdb output:

Breakpoint 1, _receive (pkt=0x1fffe1b8 <_pktbuf+44>) at /home/jfischer/src/RIOT/sys/net/gnrc/network_laye
r/ipv6/gnrc_ipv6.c:729
1: x/i $pc
=> 0x2b2a <_receive+98>:        ldr     r3, [r3, #52]   ; 0x34
(cm4-gdb) print pkt->type
$12 = GNRC_NETTYPE_IPV6
(cm4-gdb) print pkt->next->type
$13 = GNRC_NETTYPE_UDP
(cm4-gdb) print pkt->next->next->type
$14 = GNRC_NETTYPE_IPV6
(cm4-gdb) print pkt->next->next->next->type
$15 = GNRC_NETTYPE_NETIF

(cm4-gdb) print pkt      
$16 = (gnrc_pktsnip_t *) 0x1fffe1b8 <_pktbuf+44>

(cm4-gdb) print pkt->next->next      
$17 = (struct gnrc_pktsnip *) 0x1fffe1e8 <_pktbuf+92>

sorry, do not have time to dig deeper, possibly tonight

kaspar030 · 2015-12-18T10:51:44Z

As today is release day, I propose disabling NHC for this release. It seems a little to hotfixed to me. What do you guys think?

OlegHahm · 2015-12-18T11:27:28Z

What do you mean by disabling it? And be aware that without NHC no UDP connection between RIOT and Linux is possible over 6LoWPAN (at least not unless you know how to disable IPHC on Linux).

kaspar030 · 2015-12-18T11:38:24Z

What do you mean by disabling it?

I thought about disabling the pseudomodule by just not making it a dependency of IPHC.

And be aware that without NHC no UDP connection between RIOT and Linux is possible over 6LoWPAN (at least not unless you know how to disable IPHC on Linux).

Hm. With the broken NHC no UDP exchange is possible when RIOT translates between IPv6 and 6lowpan. Isn't that the border router use case?

No hard feelings here, but IMHO not having NHC is not a show stopper, and once we have a fix, we could backport (and then reenable NHC).

What does the RFC say? Isn't NHC optional? Or all of IPHC? (Sending broken packages is definately just wrong.)

The best would be to just fix it. @authmillenon? ;)

OlegHahm · 2015-12-18T11:49:04Z

RFC6282 says:

A compliant implementation of [RFC4944] as updated by this document
MUST be able to properly process a packet received that makes use of
the provisions of this document.

I still haven't fully understand which setups are currently broken.

May be a bit weird to introduce a new feature to the bug-fixing branch. However, I guess it's our decision what is allowed to go into this branch and we don't have to limit ourselves to bugfixes.

kaspar030 · 2015-12-18T12:25:21Z

I still haven't fully understand which setups are currently broken.

I stumbled upon it in this setup: linux <-> 6lr <-> RIOT, with linux<->6lr connected over ethernet (using ethos, but that shouldn't matter), and the riot nodes being SAM R21s. pinging works perfectly, but a CoAP reply from RIOT to Linux is broken. &lr <-> RIOT is sixlowpan, linux<->6lr is IPv6. Kaspar

emmanuelsearch · 2015-12-18T14:35:03Z

planning to test with the below setup as soon as I get libcoap installed on the (slow) pi.

Linux (Pi + 802.15.4 module) <=> RIOT (samr21-xpro)

jfischer-no · 2015-12-18T14:39:12Z

linux (Linux 4.4.0) <-> riot (master), udp with nhc working fine in both directions !

miri64 · 2015-12-18T14:49:54Z

What about #4485 (comment) then?

jfischer-no · 2015-12-18T14:59:45Z

About: linux (Linux 4.4.0) <-> riot (00b64b2)

miri64 · 2015-12-18T15:01:34Z

Ah sorry, did not saw the master. But the headers are still reversed RIOT internally?

OlegHahm · 2015-12-18T15:12:38Z

linux (Linux 4.4.0) <-> riot (master), udp with nhc working fine in both directions !

Also for the CoAP example?

cgundogan · 2015-12-18T15:24:54Z

I just tried the same setup as before: linux (copper) <-> 6lbr (RIOT) <-> 6lr (RIOT). I did a CoAP ping from linux to the 6lr and the fix proposed here merged into master. CoAP ping does not work. If I disable ifconfig <x> -iphc IPHC then CoAP ping works again.

OlegHahm · 2015-12-18T15:29:47Z

What about Linux (copper or libcoap) <-> 6lr (RIOT)?

cgundogan · 2015-12-18T16:02:38Z

What about Linux (copper or libcoap) <-> 6lr (RIOT)?

I have no setup for this ready. @emmanuelsearch were you able to install the coap-client?

miri64 · 2015-12-18T16:02:42Z

Found cause of #4485 (comment) will fix, but test first

emmanuelsearch · 2015-12-18T16:04:17Z

yes I have finally managed to installe coap-client (libcoap) on our local pi. Stay tuned.

emmanuelsearch · 2015-12-18T16:08:45Z

EDIT: the below works for master, but not with this PR

Seems to work for the setup I tried: Linux (Pi + 802.15.4 module) <=> RIOT (samr21-xpro)
I get the below:

pi@raspberrypi ~/libcoap $ coap-client coap://[fe80::5855:5c7c:8647:1e2e%lowpan0]/riot/board
v:1 t:CON c:GET i:22af {} [ ]
samr21-xpro

jfischer-no · 2015-12-18T16:11:16Z

tested with libcoap:
linux (Linux 4.4.0 + Debian on Phytec Wega SBC + CC2520) <-> riot (master,microcoap_server), coap with nhc works in both directions.

cgundogan · 2015-12-18T16:12:54Z

So, then the problem is only existent when ahving an 6lbr in between? At least it didn't work for me and from what I understand it also didn't work for @kaspar030 (wit this fix included in master?)

miri64 · 2015-12-18T16:36:10Z

Found another issue causing #4462 that is unrelated to this fix.

miri64 · 2015-12-22T11:39:45Z

Rebased to current master.

jfischer-no · 2015-12-22T15:41:47Z

tested quickly:

linux (Linux 4.4.0 + Debian on Phytec Wega SBC + CC2520) <-> riot-node (b398941,microcoap_server), coap 👍
linux (Debian) <-> riot 6lo (b398941) <-> riot-node (b398941,microcoap_server), coap 👎 (node sends malformed packet)

I'll try to debug, if I have time today evening...

miri64 · 2015-12-22T15:44:53Z

Might not be related to this PR, but thanks

miri64 · 2015-12-22T15:46:08Z

Updated the doc for the pkt parameter also.

jfischer-no · 2015-12-22T15:50:11Z

Might not be related to this PR, but thanks

To which PR is it related then?

miri64 · 2015-12-22T15:55:17Z

There is none yet, issue #4462 exists for that.

miri64 · 2015-12-22T15:55:42Z

This PR just fixes the packet order

cgundogan · 2015-12-22T16:09:49Z

Could you squash, please?

cgundogan · 2015-12-22T16:09:56Z

Could you squash, please?

miri64 · 2015-12-22T16:14:06Z

Done

cgundogan · 2015-12-22T16:16:24Z

I see two commits with the same commit message

miri64 · 2015-12-22T16:17:26Z

Look again ;-)

cgundogan · 2015-12-22T16:17:48Z

magic.. (: Let's wait for travis now and then merge this

miri64 · 2015-12-22T17:23:14Z

You forgot to set the label ;-)

cgundogan · 2015-12-22T19:53:01Z

travis agrees - GO

gnrc_sixlowpan: Fix IPHC/NHC packet order problem

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Dec 15, 2015

miri64 assigned jfischer-no Dec 15, 2015

miri64 added this to the Release 2015.12 milestone Dec 15, 2015

OlegHahm added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Dec 15, 2015

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

miri64 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 22, 2015

miri64 added 2 commits December 22, 2015 17:14

gnrc_sixlowpan: Fix IPHC/NHC packet order problem

d342d35

gnrc_udp: fix false assumed packet order

21beaa7

miri64 force-pushed the gnrc_sixlowpan/fix/nhc-iphc branch from 7232eea to 21beaa7 Compare December 22, 2015 16:17

jfischer-no mentioned this pull request Dec 22, 2015

IPHC/NHC broken #4462

Closed

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 22, 2015

cgundogan added a commit that referenced this pull request Dec 22, 2015

Merge pull request #4485 from authmillenon/gnrc_sixlowpan/fix/nhc-iphc

17e21e3

gnrc_sixlowpan: Fix IPHC/NHC packet order problem

cgundogan merged commit 17e21e3 into RIOT-OS:master Dec 22, 2015

This was referenced Dec 22, 2015

sixlowpan: iphc: remove dispatch and assign result to pkt #4539

Merged

gnrc_sixlowpan: Fix IPHC/NHC packet order problem (backport) #4541

Merged

cgundogan removed Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 22, 2015

miri64 mentioned this pull request Dec 23, 2015

Release 2015.12 - RC2 RIOT-OS/Release-Specs#11

Closed

34 tasks

miri64 deleted the gnrc_sixlowpan/fix/nhc-iphc branch January 6, 2016 13:30

gnrc_sixlowpan: Fix IPHC/NHC packet order problem #4485

gnrc_sixlowpan: Fix IPHC/NHC packet order problem #4485

Uh oh!

Conversation

miri64 commented Dec 15, 2015

Uh oh!

OlegHahm commented Dec 15, 2015

Uh oh!

miri64 commented Dec 15, 2015

Uh oh!

jfischer-no commented Dec 15, 2015

Uh oh!

jfischer-no commented Dec 17, 2015

Uh oh!

kaspar030 commented Dec 18, 2015

Uh oh!

OlegHahm commented Dec 18, 2015

Uh oh!

kaspar030 commented Dec 18, 2015

Uh oh!

OlegHahm commented Dec 18, 2015

Uh oh!

kaspar030 commented Dec 18, 2015 via email

Uh oh!

emmanuelsearch commented Dec 18, 2015

Uh oh!

jfischer-no commented Dec 18, 2015

Uh oh!

miri64 commented Dec 18, 2015

Uh oh!

jfischer-no commented Dec 18, 2015

Uh oh!

miri64 commented Dec 18, 2015

Uh oh!

OlegHahm commented Dec 18, 2015

Uh oh!

cgundogan commented Dec 18, 2015

Uh oh!

OlegHahm commented Dec 18, 2015

Uh oh!

cgundogan commented Dec 18, 2015

Uh oh!

miri64 commented Dec 18, 2015

Uh oh!

emmanuelsearch commented Dec 18, 2015

Uh oh!

emmanuelsearch commented Dec 18, 2015

Uh oh!

jfischer-no commented Dec 18, 2015

Uh oh!

cgundogan commented Dec 18, 2015

Uh oh!

miri64 commented Dec 18, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

jfischer-no commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

jfischer-no commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

cgundogan commented Dec 22, 2015

Uh oh!

cgundogan commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

cgundogan commented Dec 22, 2015

Uh oh!

miri64 commented Dec 22, 2015

Uh oh!

cgundogan commented Dec 22, 2015

Uh oh!