ng_sixlowpan: Fixes #3494

miri64 · 2015-07-23T16:49:29Z

Fixes some issues with fragmentation, IPHC and the interface state of a 6LoWPAN interface.

jnohlgard · 2015-07-24T03:52:47Z

sys/net/network_layer/ng_sixlowpan/frag/rbuf.h

@@ -20,6 +20,7 @@
 #define NG_SIXLOWPAN_FRAG_RBUF_H_

 #include <inttypes.h>
+#include <stdbool.h>


you did not add any stdbool.h stuff?

miri64 · 2015-07-24T05:22:37Z

Adressed comments

miri64 · 2015-07-24T09:34:09Z

The only issue still open is that there is a hard fault at the receiving an uncompressed, but fragmented packet. A don't really know why that is. @gebart do you have an idea maybe?

miri64 · 2015-07-24T09:34:47Z

Rebased to current master

jnohlgard · 2015-07-24T09:50:03Z

On Jul 24, 2015 11:34 AM, "Martine Lenders" notifications@github.com
wrote:

The only issue still open is that there is a hard fault at the receiving
an uncompressed, but fragmented packet. A don't really know why that is.
@gebart do you have an idea maybe?

Do you have any more information on the hard fault? Backtrace etc.

I can look at it in 10 days, I will be away on holidays next week and will
be back again on Monday the week after that.

miri64 · 2015-07-24T10:01:22Z

Do you have any more information on the hard fault? Backtrace etc.

Yes, sorry for not giving this earlier: it's happening at [1] due to [2] somehow overwriting the pointers in entry->pkt.

I can look at it in 10 days, I will be away on holidays next week and will
be back again on Monday the week after that.

Okay, until then I hopefully have found the bug myself :-D

[1] https://github.com/RIOT-OS/RIOT/pull/3494/files#diff-a8f58d41a6d77cb7e5f310c61eae2c44R138
[2] https://github.com/RIOT-OS/RIOT/pull/3494/files#diff-a8f58d41a6d77cb7e5f310c61eae2c44R126

jnohlgard · 2015-07-24T10:21:52Z

@authmillenon some hints to find it:

put a breakpoint at rbuf.c:125
run to breakpoint
set watchpoint on entry->pkt or entry->pkt->next, (or entry->pkt->next->next etc.)
continue execution

miri64 · 2015-07-24T10:32:33Z

Oh I did that already, but to no avail… I suspect that there is either a illegal release somewhere or that the packet buffer has a really nifty bug.

jnohlgard · 2015-07-24T10:36:56Z

Did you not get any triggered watch points at all?

miri64 · 2015-07-24T10:37:32Z

Yes I did, but that was how I found out that [2] was overwriting ;-)

jnohlgard · 2015-07-24T10:40:22Z

OK, try expanding the macro for easier debugging:

do {                                                                                           \
  __typeof(entry->pkt) _tmp;                                                                        \
  (netif)->next=((void *)0);                                                                            \
  if (entry->pkt) {                                                                                  \
    _tmp = entry->pkt;                                                                               \
    while (_tmp->next) { _tmp = _tmp->next; }                                                  \
    _tmp->next=(netif);                                                                          \
  } else {                                                                                     \
    (entry->pkt)=(netif);                                                                              \
  }                                                                                            \
} while (0)

miri64 · 2015-07-24T10:48:22Z

The macro is not the problem… for some reason the ng_pktbuf_add() called in [2] overrides existing packets in the packet buffer. As I already stated this is usually due to

a bug in the packet buffer (unlikely)
usage of an already released packet (more likely)

jnohlgard · 2015-07-24T10:50:53Z

sorry, I switched the lines in my head. I agree, it is likely a packet being used after released.

miri64 · 2015-07-27T10:56:50Z

Since it works splendidly with #3496 (see my test descriptions there) I say it was a bug in the packet buffer and declare this PR is no longer WIP.

PeterKietzmann · 2015-07-27T13:04:19Z

I can confirm that this PR improves IPHC and fragmentation and does not destroy anything.

OlegHahm · 2015-07-27T13:42:08Z

sys/net/network_layer/ng_ipv6/netif/ng_ipv6_netif.c


+ if (ng_netapi_get(ifs[i], NETCONF_OPT_MAX_PACKET_SIZE,
+ 0, &max_frag_size, sizeof(max_frag_size)) < 0) {
+ /* if error we assume it works */


Wouldn't it make sense to set the max. packet size to more sensible size than 2^16-1 in case that this fails? E.g. 1280 bytes.

Why is 1280 as the minimum MTU more sensible?

Because RFC2460 requires a minimum MTU of 1280 bytes.

Yes, but why take the minimum, when the maximum is just as good. My point is: I don't see the point where 1280 bytes is a more sensible number than UINT16_MAX or 4242 or 2015 or 5555 [edit] as long as it stays in the interval [1280, 2^16-1]. The number is quite arbitrary so I decided for the maximum a (standard) IPv6 frame can carry (since its length field is 16-bit long).

Also just to be clear: this value is not the MTU, this is just the number of bytes 6LoWPAN decides to fragment a packet. So even smaller values than 1280 are possible (and are usually chosen with IEEE 802.15.4 devices ;-)) 6LoWPAN's maximum MTU is max(max_L2_packet_size, 2^11 - 1), because the datagram_length field of the fragmentation header is 11 bit long (but nothing stops it to send bigger packets unfragmented if the link-layer supports that).

(Furthermore, this is not a new change, I just moved this from [1] since the ifconfig 7 [-]iphc command then only would work after the first 6LoWPAN frame would have been send).

I just think that it is more likely for a link layer to being able to carry 1--2kBytes instead of being able to carry 65kBytes payload.

And yes, I lost track of what this PR is essentially doing. I cannot do a proper review (without reading and understanding the whole fragmentation code again). So, I would say, if no one else is willing to review, let's trust the outcome, which seems to be good according to @PeterKietzmann.

I'm not sure if we are talking over each others heads, but this line does not set anything to the device, but reads the maximum frame length of the link layer into max_frag_size. UINT16_MAX is only used iff the link layer does not support this option.

Yes, that's what I understood. And I think a sensible default value is more in the range of an typical Ethernet frame than in the range of 65kBytes.

I just think that it is more likely for a link layer to being able to carry 1--2kBytes instead of being able to carry 65kBytes payload.

and

Yes, that's what I understood. And I think a sensible default value is more in the range of an typical Ethernet frame than in the range of 65kBytes.

Well the IPv6 layer usually knows very well about the (in case of 6LoWPAN perceived) MTU of the device, so packets that are bigger than the device can handle would never reach 6LoWPAN anyways (further argument why to take the maximum rather than the minimum, because it makes all checks to fragment a packet false). Ethernet and IEEE 802.15.4 aren't the only link-layers out there, afaik BLE provides fragmentation itself, making its maximum "frame" size pretty big in theory.

And yes, I lost track of what this PR is essentially doing. I cannot do a proper review (without reading and understanding the whole fragmentation code again). So, I would say, if no one else is willing to review, let's trust the outcome, which seems to be good according to @PeterKietzmann.

😁👍

Well the IPv6 layer usually knows very well about the (in case of 6LoWPAN perceived) MTU of the device

How does it do so?

If this value is only used for 6LoWPAN fragmentation, I would vote for either exiting here or setting this value to a fixed size.

How does it do so?

Either by being set by the user (though there is arguably no command for this yet) or by getting it from the device. If both are not the case the default value is 1280.

If this value is only used for 6LoWPAN fragmentation, I would vote for either exiting here or setting this value to a fixed size.

I do the later here ;-). But seriously, after this discussion and reviewing what happens to a packet as it traverses the stack I do think UINT16_MAX is the most sensible solution. Say the link layer has no maximum packet length because it supports streaming. Then UINT16_MAX as the size of a single maxed out IPv6 packet (even that is not true because the header is missing from that the) Is the maximum fragment size 6LoWPAN can send. If not it either can tell both IPv6 and 6LoWPAN this through this option or it is always 1280 (as you proposed earlier) which all will fail the "should I fragment" test because they are all lesser than UINT16_MAX.

OlegHahm · 2015-07-27T16:27:33Z

Either by being set by the user (though there is arguably no command for this yet)

Let's assume in IoT (M2M) there is no user/administrator.

or by getting it from the device.

Isn't the netapi call exactly doing this?

Say the link layer has no maximum packet length because it supports streaming.

Are you sure that any link layer supports fragmentation of arbitrary payload?

Anyway, is this value only used for 6LoWPAN or also for other parts of IPv6?

miri64 · 2015-07-27T16:48:32Z

Let's assume in IoT (M2M) there is no user/administrator

As I've stated before: then IPv6 defaults to its minimum MTU (1280) unless the device is willing to tell it itself ;)

Are you sure that any link layer supports fragmentation of arbitrary payload?

No.

Anyway, is this value only used for 6LoWPAN or also for other parts of IPv6?

Currently, this is only used by 6LoWPAN, but nothing stops us from using this also for 6LoWPANZ or other 6lo specifications.

OlegHahm · 2015-07-27T17:12:52Z

Ok, let's rewind to the original purpose of this variable: it is meant for (at least theoretical) any convergence layer (e.g. 6LoWPAN) below IPv6 to tell this layer the maximum size of a packet on the link layer, right? If the device cannot tell the convergence layer this size, I would expect the implementation to throw an error here.

PeterKietzmann · 2015-07-28T14:15:43Z

I don't dare to ACK this PR but still I'd like to see this merged soon, as it really fixes some issues with fragmentation and IPHC. Who is willing to give an ACK :-) ? Are we ready to squash and run Travis?

OlegHahm · 2015-07-28T16:08:54Z

Please squash. Will review.

cgundogan · 2015-07-28T16:45:30Z

sys/net/network_layer/ng_sixlowpan/frag/rbuf.c

@@ -117,7 +105,7 @@ void rbuf_add(ng_netif_hdr_t *netif_hdr, ng_sixlowpan_frag_t *frag,
 }

 while (ptr != NULL) {
- if (_rbuf_int_in(ptr, offset, offset + dg_frag_size - 1)) {
+ if (_rbuf_int_in(ptr, offset, offset + frag_size - 1)) {
 DEBUG("6lo rfrag: overlapping or same intervals, discarding datagram\n");
 ng_pktbuf_release(entry->pkt);
 _rbuf_rem(entry);


_rbuf_rem sets entry->pkt to NULL. Is this a desired behaviour? What if this pkt has several users?

What if this pkt has several users?

This is not of relevance for this module, since the other user then should have its own reference.

Normally this module should be the packet's only user, since it creates it and in this case disregards it because of an error. Only if the packet was reassembled successfully it will be handed over to another thread.

miri64 · 2015-07-28T18:25:39Z

Rebased and squashed

BytesGalore · 2015-07-28T19:23:39Z

sys/net/network_layer/ng_ipv6/netif/ng_ipv6_netif.c


+ if (ng_netapi_get(ifs[i], NETCONF_OPT_MAX_PACKET_SIZE,


I guess this needs to be changed to NETOPT_MAX_PACKET_SIZE

NG_ prefixed of course

Jupp fixed already

miri64 · 2015-07-28T19:30:08Z

Found a little merge-error.

OlegHahm · 2015-07-28T20:20:56Z

Everything's broken: ACK

cgundogan · 2015-07-28T20:21:13Z

Everything's broken: ACK

👍 ACk

PeterKietzmann · 2015-07-28T20:32:36Z

😱 ?!?!

ng_sixlowpan: Fixes

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet NSTF labels Jul 23, 2015

miri64 assigned jnohlgard Jul 23, 2015

miri64 added this to the Release 2015.06 milestone Jul 23, 2015

miri64 mentioned this pull request Jul 23, 2015

shell: ifconfig: add capability to (un)set IPHC #3493

Merged

jnohlgard reviewed Jul 24, 2015
View reviewed changes

miri64 force-pushed the ng_sixlowpan_frag/fix/rbuf branch from 75ea011 to d533aa4 Compare July 24, 2015 09:34

miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 27, 2015

miri64 changed the title ~~ng_sixlowpan: Fixes [WIP]~~ ng_sixlowpan: Fixes Jul 27, 2015

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jul 27, 2015

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

OlegHahm reviewed Jul 27, 2015
View reviewed changes

cgundogan reviewed Jul 28, 2015
View reviewed changes

miri64 force-pushed the ng_sixlowpan_frag/fix/rbuf branch 2 times, most recently from 54337e2 to f880280 Compare July 28, 2015 18:25

cgundogan mentioned this pull request Jul 28, 2015

ng_rpl: auto address configuration #3489

Merged

BytesGalore reviewed Jul 28, 2015
View reviewed changes

miri64 force-pushed the ng_sixlowpan_frag/fix/rbuf branch from f880280 to 524a4e8 Compare July 28, 2015 19:29

miri64 force-pushed the ng_sixlowpan_frag/fix/rbuf branch from 524a4e8 to 26c98b8 Compare July 28, 2015 19:39

miri64 added 3 commits July 28, 2015 22:07

ng_sixlowpan_frag: fixes

dc2113d

ng_{sixlowpan|ipv6}_netif: don't create ng_sixlowpan_netif_ts adhoc

c6fb261

ng_sixlowpan: DEBUG fixes

cc298d8

miri64 force-pushed the ng_sixlowpan_frag/fix/rbuf branch from 26c98b8 to cc298d8 Compare July 28, 2015 20:07

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Jul 28, 2015

miri64 added a commit that referenced this pull request Jul 28, 2015

Merge pull request #3494 from authmillenon/ng_sixlowpan_frag/fix/rbuf

8960be1

ng_sixlowpan: Fixes

miri64 merged commit 8960be1 into RIOT-OS:master Jul 28, 2015

miri64 deleted the ng_sixlowpan_frag/fix/rbuf branch July 28, 2015 20:36

miri64 mentioned this pull request Jul 28, 2015

ng_sixlowpan: fix the kaputt-reviewed fixes #3516

Merged

miri64 added the Area: network Area: Networking label Sep 30, 2018

ng_sixlowpan: Fixes #3494

ng_sixlowpan: Fixes #3494

Conversation

miri64 commented Jul 23, 2015

Choose a reason for hiding this comment

miri64 commented Jul 24, 2015

miri64 commented Jul 24, 2015

miri64 commented Jul 24, 2015

jnohlgard commented Jul 24, 2015

miri64 commented Jul 24, 2015

jnohlgard commented Jul 24, 2015

miri64 commented Jul 24, 2015

jnohlgard commented Jul 24, 2015

miri64 commented Jul 24, 2015

jnohlgard commented Jul 24, 2015

miri64 commented Jul 24, 2015

jnohlgard commented Jul 24, 2015

miri64 commented Jul 27, 2015

PeterKietzmann commented Jul 27, 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

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 Jul 27, 2015

miri64 commented Jul 27, 2015

OlegHahm commented Jul 27, 2015

PeterKietzmann commented Jul 28, 2015

OlegHahm commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Jul 28, 2015

OlegHahm commented Jul 28, 2015

cgundogan commented Jul 28, 2015

PeterKietzmann commented Jul 28, 2015