gnrc_sixlowpan_iphc: fix _compressible() by miri64 · Pull Request #10950 · RIOT-OS/RIOT

miri64 · 2019-02-05T14:26:02Z

Contribution description

When either gnrc_sixlowpan_iphc_nhc or gnrc_udp is not compiled in _compressible() never returns true. This causes the dispatch snip in gnrc_sixlowpan_iphc_send() to be of length 0, meaning dispatch->data is NULL, causing possible crashes when trying to send IPv6 packets over 6LoWPAN without NHC or UDP.

Testing procedure

tests/gnrc_tcp_server and tests/gnrc_tcp_client should work on a 6Lo-based board.
examples/gnrc_networking should still be able to exchange UDP packets. Those packets should be compressed with NHC (check with sniffer).

Issues/PRs references

Fixes #10947

When either `gnrc_sixlowpan_iphc_nhc` or `gnrc_udp` is not compiled in `_compressible()` never returns `true`. This causes the `dispatch` snip in `gnrc_sixlowpan_iphc_send()` to be of length 0, meaning `dispatch->data` is `NULL`, causing possible crashes when trying to send IPv6 packets over 6LoWPAN without NHC or UDP.

miri64 · 2019-02-05T14:32:05Z

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

        case GNRC_NETTYPE_IPV6:
 #if defined(MODULE_GNRC_SIXLOWPAN_IPHC_NHC) && defined(MODULE_GNRC_UDP)
        case GNRC_NETTYPE_UDP:
-            return true;


@cgundogan I remember some discussion about this line, I believe with you... however this exists since 0a9793c, so I'm not sure when and where this happened.

hm, tbh I do not remember such a discussion, but the change is clearly an improvement ..

Ok, then we agree, that this change fixes things :-)

PeterKietzmann · 2019-02-05T16:05:35Z

Tested the following with this PR:

native - native
gnrc_networking
ping6 1000 <addr>1000 0 0: ok
udp send <addr> 1234 1234567890: ok

Wireshark dump looks ok (no 6Lo of course)

samr21-xpro - samr21-xpro
gnrc_networking
ping6 1000 <addr>1000 0 0: ok
udp send <addr> 1234 1234567890: ok

Wireshark dump looks ok, see

gnrc_tcp_*
Both applications build and run on a samr21-xpro. But they don't work as expected. Only thing I see is Router Solicitations from both nodes. No Neighbor Discovery, no nothing. Is there an other module missing?

miri64 · 2019-02-05T16:07:51Z

gnrc_tcp_*
Both applications build and run on a samr21-xpro. But they don't work as expected. Only thing I see is Router Solicitations from both nodes. No Neighbor Discovery, no nothing. Is there an other module missing?

First of all, we need to make sure they ever worked on this board? If no, and they don't crash this is non-blocking as it has nothing to do with this PR (but might still have to do with #10947).

miri64 · 2019-02-05T16:13:05Z

Of course it doesn't work. The client tries to connect to fe80::affe which 6Lo neighbor discovery reverse translates to a link-layer address. Since fe80::affe is not based on a link-layer address next-hop determination fails => the TCP client can't connect.

miri64 · 2019-02-05T16:18:34Z

@PeterKietzmann can you try with the TCP_TARGET_ADDR environment variable set to the auto-configured link-layer address of the server?

PeterKietzmann · 2019-02-05T16:26:24Z

Already tried that, without success. But I can't even get it to work on 2018.07 release branch. I agree that it's unrelated to this PR but it would've been great if it was clarified in here.

PeterKietzmann · 2019-02-05T16:34:39Z

(btw: yes packets are dropped in gnrc_ipv6 due to missing NC entry)

brummer-simon · 2019-02-05T16:36:01Z

gnrc_tcp_*
Both applications build and run on a samr21-xpro. But they don't work as expected. Only thing I see is Router Solicitations from both nodes. No Neighbor Discovery, no nothing. Is there an other module missing?

First of all, we need to make sure they ever worked on this board? If no, and they don't crash this is non-blocking as it has nothing to do with this PR (but might still have to do with #10947).

I developed and test gnrc_tcp mostly on the samr21-xpro. Both application worked.

PeterKietzmann · 2019-02-05T16:37:59Z

So I'm gonna open a separate issue. ACK for the fix in this PR

miri64 · 2019-02-05T16:45:32Z

I just tested the gnrc_tcp tests for native. There I get a ECONNRESET from the server :-/.

PeterKietzmann · 2019-02-05T16:49:29Z

That's an other issue. Please read #10945 carefully. If you start the client shortly before the server, it should directly work.

I'm gonna open a separate issue for the missing ND.

miri64 · 2019-02-05T16:51:02Z

I'm gonna open a separate issue for the missing ND.

The ND is not missing. The test is using it wrong.

miri64 · 2019-02-05T16:51:42Z

That's an other issue. Please read #10945 carefully. If you start the client shortly before the server, it should directly work.

Didn't know about that issue yet ;-).

PeterKietzmann · 2019-02-05T16:54:16Z

The ND is not missing. The test is using it wrong.

Then how about you open a new issue? Because all I see is missing ND, an empty NIB and I don't really know how a correct usage would look like.

brummer-simon · 2019-02-05T16:54:30Z

I'm gonna open a separate issue for the missing ND.

The ND is not missing. The test is using it wrong.

How to use it right then? I mean the test are crappy currently but I want to keep them alive until I find the time to replace them.

miri64 · 2019-02-05T17:25:34Z

I don't know. I had to look into what's going on there exactly. However, since they are the only networking-related tests having problems ATM, my assessment is rather that something weird is happening there, than in the networking layer.

brummer-simon · 2019-02-05T17:55:28Z

Okay well then,

the highest priority for the existing TCP test should be, cause as less problems as possible.
You mentioned that link local addresses could cause problems. What prefix would you suggest to use for testing then?

Secondly, I am working more or less frequently on replacing the existing gnrc_tcp tests.
This is the feature branch: https://github.com/brummer-simon/RIOT/tree/gnrc_tcp-improve_tests
Would you take a look sometime to check if am on the right track? You have clearly more experience in the automation of network tests.

miri64 · 2019-02-05T18:20:04Z

Backport provided in #10954

miri64 · 2019-02-05T18:22:15Z

What prefix would you suggest to use for testing then?

Link-local addresses are fine. Everything else would require even more network bootstrapping (configuring routes etc). However, the suffix should be based on the link-local address for 6Lo-based networks (see RFC 6775 for why that is).

Would you take a look sometime to check if am on the right track? You have clearly more experience in the automation of network tests.

Will do when I find the time.

miri64 added this to the Release 2019.01 milestone Feb 5, 2019

miri64 requested a review from PeterKietzmann February 5, 2019 14:26

miri64 mentioned this pull request Feb 5, 2019

tests/gnrc_tcp_*: not working on 6Lo-based boards #10947

Closed

miri64 requested a review from cgundogan February 5, 2019 14:30

miri64 commented Feb 5, 2019

View reviewed changes

PeterKietzmann approved these changes Feb 5, 2019

View reviewed changes

PeterKietzmann merged commit c04adf6 into RIOT-OS:master Feb 5, 2019

miri64 deleted the gnrc_sixlowpan_iphc/fix/compressible branch February 5, 2019 16:45

miri64 mentioned this pull request Feb 5, 2019

gnrc_sixlowpan_iphc: fix _compressible() [backport 2019.01] #10954

Merged

This was referenced Feb 6, 2019

net/gnrc_tcp: timeout of connection establishment #10945

Closed

tests/gnrc_tcp_*: #2 not working on 6Lo-based boards #10956

Closed

Conversation

miri64 commented Feb 5, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

cgundogan Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

brummer-simon commented Feb 5, 2019

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

PeterKietzmann commented Feb 5, 2019

Uh oh!

brummer-simon commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

brummer-simon commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

miri64 commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants