Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gnrc_sixlowpan_iphc: fix _compressible() #10950

Merged
merged 1 commit into from
Feb 5, 2019
Merged

gnrc_sixlowpan_iphc: fix _compressible() #10950

merged 1 commit into from
Feb 5, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 5, 2019

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

@miri64
gnrc_sixlowpan_iphc: fix _compressible()
43c2c72 
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 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Feb 5, 2019
@miri64 miri64 added this to the Release 2019.01 milestone Feb 5, 2019
@miri64 miri64 requested a review from PeterKietzmann February 5, 2019 14:26
@miri64 miri64 mentioned this pull request Feb 5, 2019
Closed
@miri64 miri64 requested a review from cgundogan February 5, 2019 14:30
miri64
miri64 commented Feb 5, 2019
View reviewed changes
sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c
@@ -647,8 +647,8 @@ static inline bool _compressible(gnrc_pktsnip_t *hdr)
case GNRC_NETTYPE_IPV6:
#if defined(MODULE_GNRC_SIXLOWPAN_IPHC_NHC) && defined(MODULE_GNRC_UDP)
case GNRC_NETTYPE_UDP:
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PeterKietzmann
Copy link
Member

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
udp_samr21

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
Copy link
Member Author

miri64 commented Feb 5, 2019

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
Copy link
Member Author

miri64 commented Feb 5, 2019

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
Copy link
Member Author

miri64 commented Feb 5, 2019

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

@PeterKietzmann
Copy link
Member

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
Copy link
Member

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

@brummer-simon
Copy link
Member

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
Copy link
Member

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

@PeterKietzmann PeterKietzmann merged commit c04adf6 into RIOT-OS:master Feb 5, 2019
@miri64 miri64 deleted the gnrc_sixlowpan_iphc/fix/compressible branch February 5, 2019 16:45
@miri64
Copy link
Member Author

miri64 commented Feb 5, 2019

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

@PeterKietzmann
Copy link
Member

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
Copy link
Member Author

miri64 commented Feb 5, 2019

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

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

@miri64
Copy link
Member Author

miri64 commented Feb 5, 2019

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

miri64 commented Feb 5, 2019

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
Copy link
Member

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 miri64 mentioned this pull request Feb 5, 2019
Merged
@miri64
Copy link
Member Author

miri64 commented Feb 5, 2019

Backport provided in #10954

@miri64
Copy link
Member Author

miri64 commented Feb 5, 2019

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.

This was referenced Feb 6, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.

tests/gnrc_tcp_*: not working on 6Lo-based boards
4 participants
@miri64 @PeterKietzmann @brummer-simon @cgundogan