fix@nhc-on-br #4544

jfischer-no · 2015-12-23T14:42:29Z

This PR fixes the UDP transmission from a RIOT-br to a RIOT-node.
The problem is that the forwarded packet has no nested structure (such as a packet generated on the node, for example, pkt->next-> next-> type == GNRC_NETTYPE_UDP).

fix for testing, see #4462
Tested on PhyNode(pba-d-01-kw2x) as BR and PhyNode as node (microcoap_server).

… equal to aligned_size

miri64 · 2015-12-23T14:53:15Z

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

-    gnrc_pktbuf_realloc_data(udp, nhc_len);
+
+    if (udp->type == GNRC_NETTYPE_IPV6) {
+        /* forwarded ipv6 packet */


Wouldn't it be better to mark the UDP header properly before encoding in this case?

Had it in my head, I think yes.

jfischer-no · 2015-12-23T15:32:35Z

e78dbd3 commit is misleading, (pkt->size - aligned_size) will overflow if _align(size) is greater then pkt->size.
e.g.

size = 21; // new size
aligned_size = 24;
pkt_size = 23;

I will open a own PR for e78dbd3 tonight.

cgundogan · 2015-12-23T17:03:55Z

@jfischer-phytec-iot just tested this. CoAP seems to work for my setup with your fix. I can CoAP ping the 6lr and I can request "/riot/board". Requesting "/.well-known/core" however does not work very reliably. What about you?

cgundogan · 2015-12-23T17:05:53Z

requesting well-known does only seem to work for the first time, then I need to reboot the 6lr to request it again

cgundogan · 2015-12-23T17:07:29Z

this can also be an unrelated issue though.

jfischer-no · 2015-12-23T17:28:13Z

I have only tested "/riot/board". I had to interrupt ... 🔍 -> 🎁. I will continue tonight.

cgundogan · 2015-12-23T19:05:40Z

It seems that CoAP requests do not work if they are big enough to get fragmented by 6lo. Requesting /riot/board works, because its response is too small to get fragment. I implemented a custom coap handler that returns a large string that enforces fragmenting and again - the same behaviour as for the well-known request: it does not work. Sniffing on the linux side reveals that the response (garbled or not) is not received. So the packet gets lost between the 6lr and 6lbr.

jfischer-no · 2015-12-24T08:35:37Z

@cgundogan Do ping6 with increased payload works?

#!/bin/bash

IP=affe::42 # a riot node behind 6lbr

for (( n=0; n<=13; n++ ))
do
        for (( packetsize=0; packetsize<=299; packetsize++ ))
        do
           echo "ping6 with payload $packetsize"
           ping6 -i 0.2 -c 1 -s $packetsize -w 1 $IP
        done

        for (( packetsize=299; packetsize>0; packetsize-- ))
        do
           echo "ping6 with payload $packetsize"
           ping6 -i 0.2 -c 1 -s $packetsize -w 1 $IP
        done
done

jfischer-no · 2015-12-30T09:21:10Z

@cgundogan @authmillenon For fragmented packets, rbuf_add fails after gnrc_sixlowpan_iphc_decode, because entry->pkt points to udp header.(?)
https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/sixlowpan/frag/rbuf.c#L114
I do not know yet how is it better to fix.

kaspar030 · 2015-12-30T11:16:11Z

Hm, so with this PR, NHC messes up fragmentation? Maybe drop NHC for the release? Taking a while to fix...

jfischer-no · 2015-12-30T11:56:01Z

with this PR, NHC messes up fragmentation

jaein, it was created by me, I missed it in the implementation.
Also, it pleases me not as it currently works with the pktsnip and NHC (UDP). It is difficult to understand and there are other NHCs to implement not only for UDP.

@OlegHahm @cgundogan @kaspar030 @authmillenon
I am in favor to drop NHC for the release, but merge 7350062 and e78dbd3.

miri64 · 2015-12-30T23:42:12Z

@jfischer-phytec-iot are you sure it is a UDP snip? entry->pkt should have a structure like this:

"Payload" might be marked as UDP.

jfischer-no · 2015-12-31T11:25:23Z

@authmillenon no, not at https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/sixlowpan/frag/rbuf.c#L114
There are (for first frag, on BR):

entry->pkt(type=GNRC_NETTYPE_UNDEF||GNRC_NETTYPE_UDP, size=8)
    |->next(type=GNRC_NETTYPE_SIXLOWPAN, size=...)
    L->next=NULL

I think gnrc_pktbuf_realloc_data for L2 pktsnip would be better method than add new pktsnip for udp header.

miri64 · 2015-12-31T13:03:55Z

You mean for IPv6 header?

miri64 · 2015-12-31T13:04:35Z

And that's totally broken indeed. entry->pkt should point to an already decoded IPv6 header too.

miri64 · 2015-12-31T13:05:54Z

But given my implementation of IPv6 and of 6LoWPAN IPHC I guess reallocing the IPv6 header was what I had originally in mind for NHC.

miri64 · 2016-01-04T11:10:45Z

sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c

+        if (_align(pkt->size) > aligned_size) {
+            _pktbuf_free(((uint8_t *)pkt->data) + aligned_size,
+                         pkt->size - aligned_size);
+        }


I'm not sure if this could generate fragmentation in the packet buffer. When is this part free'd otherwise?

@authmillenon moved to #4602

cgundogan · 2016-01-04T11:38:40Z

@jfischer-phytec-iot ping6 with bigger payload does work with your PR merged into master. However, CoAP still breaks if packets get fragmented.

cgundogan · 2016-01-04T17:55:44Z

I will label this PR for the next release, because I don't think that this will be fixed within the following days. @authmillenon @jfischer-phytec-iot opinions?

miri64 · 2016-01-26T18:22:39Z

👍

jnohlgard · 2016-03-01T15:39:35Z

@jfischer-phytec-iot What is the status of this PR?

jnohlgard · 2016-03-12T10:43:19Z

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

-    /* shrink udp allocation to final size */
-    gnrc_pktbuf_realloc_data(udp, nhc_len);
+
+    if (udp->type == GNRC_NETTYPE_IPV6) {


shouldn't this be if (udp->type != GNRC_NETTYPE_UDP) {?

I don't know, since I honestly don't know what state the packet is in at this point.

miri64 · 2016-03-16T11:45:50Z

I think this PR needs a rebase.

miri64 · 2016-03-16T11:46:17Z

(several PRs mentioned in this PR were merged, though there is no merge conflict).

jnohlgard · 2016-03-16T12:29:53Z

@authmillenon what do you think about my comment above?

shouldn't this be if (udp->type != GNRC_NETTYPE_UDP) {?

I can adopt this PR and rebase on master if you want?

miri64 · 2016-03-16T12:42:00Z

@authmillenon what do you think about my comment above?

shouldn't this be if (udp->type != GNRC_NETTYPE_UDP) {?

Gave an answer ;-)

I can adopt this PR and rebase on master if you want?

@jfischer-phytec-iot are you okay with that?

miri64 · 2016-03-23T13:43:48Z

@jfischer-phytec-iot seems to be inactive at the moment. @gebart can you please adapt for the release?

jnohlgard · 2016-03-23T15:46:24Z

I'm off on holidays at the moment, but I might be able to do this during
the weekend anyway.
On Mar 23, 2016 2:43 PM, "Martine Lenders" [email protected] wrote:

@jfischer-phytec-iot https://github.com/jfischer-phytec-iot seems to be
inactive at the moment. @gebart https://github.com/gebart can you
please adapt for the releases

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4544 (comment)

miri64 · 2016-03-31T22:41:10Z

Postponed for next release.

OlegHahm · 2016-04-01T19:59:44Z

Why postponing? I think this is important.

miri64 · 2016-04-01T21:55:56Z

But nobody is able to work on it at the moment as it seems...

jnohlgard · 2016-04-02T06:10:05Z

posted a rebased PR #5232

miri64 · 2016-04-20T05:21:54Z

#5232 was merged, so I close this one

jfischer-no added 2 commits December 23, 2015 15:37

gnrc_sixlowpan_iphc.c: handle forwarded GNRC_NETTYPE_IPV6 packet

7350062

gnrc_pktbuf_static.c: do not free pktbuf if pkt->size is less then or…

e78dbd3

… equal to aligned_size

jfischer-no assigned miri64 Dec 23, 2015

jfischer-no added this to the Release 2015.12 milestone Dec 23, 2015

miri64 reviewed Dec 23, 2015
View reviewed changes

jfischer-no mentioned this pull request Dec 23, 2015

IPHC/NHC broken #4462

Closed

miri64 mentioned this pull request Dec 23, 2015

sixlowpan: iphc: fix dependencies for udp and nhc (backport) #4545

Merged

kaspar030 mentioned this pull request Jan 4, 2016

sys: net: disable NHC #4566

Merged

miri64 reviewed Jan 4, 2016
View reviewed changes

OlegHahm removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Feb 27, 2016

miri64 mentioned this pull request Mar 9, 2016

gnrc_sixlowpan_iphc_nhc: fix NHC UDP decoding for fragmented packets #4935

Merged

jnohlgard mentioned this pull request Mar 10, 2016

gnrc_sixlowpan: Preserve order when replacing 6lowpan header by decoded IPv6 header #5029

Merged

miri64 mentioned this pull request Mar 11, 2016

gnrc_sixlowpan: enable NHC by default #5044

Merged

jnohlgard added the ugly label Mar 12, 2016

jnohlgard reviewed Mar 12, 2016
View reviewed changes

jnohlgard mentioned this pull request Mar 12, 2016

gnrc_sixlowpan_frag: use IPv6 type for reassembled packet #5049

Merged

miri64 modified the milestones: Release 2016.07, Release 2016.04 Mar 31, 2016

jnohlgard mentioned this pull request Apr 2, 2016

gnrc_sixlowpan_iphc.c: handle forwarded GNRC_NETTYPE_IPV6 packet #5232

Merged

OlegHahm modified the milestones: Release 2016.04, Release 2016.07 Apr 3, 2016

miri64 closed this Apr 20, 2016

Yonezawa-T2 mentioned this pull request Oct 14, 2016

gnrc_ipv6: fix UDP issue #5926

Closed

jfischer-no deleted the fix@nhc-on-br branch January 23, 2017 09:13

fix@nhc-on-br #4544

fix@nhc-on-br #4544

Conversation

jfischer-no commented Dec 23, 2015

miri64 Dec 23, 2015

Choose a reason for hiding this comment

jfischer-no Dec 23, 2015

Choose a reason for hiding this comment

jfischer-no commented Dec 23, 2015

cgundogan commented Dec 23, 2015

cgundogan commented Dec 23, 2015

cgundogan commented Dec 23, 2015

jfischer-no commented Dec 23, 2015

cgundogan commented Dec 23, 2015

jfischer-no commented Dec 24, 2015

jfischer-no commented Dec 30, 2015

kaspar030 commented Dec 30, 2015

jfischer-no commented Dec 30, 2015

miri64 commented Dec 30, 2015

jfischer-no commented Dec 31, 2015

miri64 commented Dec 31, 2015

miri64 commented Dec 31, 2015

miri64 commented Dec 31, 2015

miri64 Jan 4, 2016

Choose a reason for hiding this comment

jfischer-no Jan 7, 2016

Choose a reason for hiding this comment

cgundogan commented Jan 4, 2016

cgundogan commented Jan 4, 2016

miri64 commented Jan 26, 2016

jnohlgard commented Mar 1, 2016

jnohlgard Mar 12, 2016

Choose a reason for hiding this comment

miri64 Mar 16, 2016

Choose a reason for hiding this comment

miri64 commented Mar 16, 2016

miri64 commented Mar 16, 2016

jnohlgard commented Mar 16, 2016

miri64 commented Mar 16, 2016

miri64 commented Mar 23, 2016

jnohlgard commented Mar 23, 2016

miri64 commented Mar 31, 2016

OlegHahm commented Apr 1, 2016

miri64 commented Apr 1, 2016

jnohlgard commented Apr 2, 2016

miri64 commented Apr 20, 2016