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.

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

Repair the 6LoWPAN IPv6 tx/rx kernel test #1065

Merged
merged 4 commits into from Jul 6, 2018

Conversation

Projects
None yet
5 participants
@hudson-ayers
Copy link
Collaborator

hudson-ayers commented Jul 2, 2018

Pull Request Overview

When I started to look into moving the 6LoWPAN stack over to the component interface, I realized that the ipv6_lowpan_test fails every time.
This pull request fixes the ipv6_lowpan_test kernel test so that following the instructions in the file leads to a successful execution of the test.
This PR includes fixes to addresses so that the receiving Imix will receive packets bound for its MAC address. This PR also includes a fix to the logic for inferring the UDP length field per RFC 6282.

This PR also fixes the UDP checksum and some issues with byte ordering.

Testing Strategy

Tested by configuring ipv6_lowpan_test on two imixes and running the test to completion, and by sniffing the packets sent by these tests and using wireshark to check the output and checksum are correct.

Documentation Updated

  • Updated the relevant comments in ipv6_lowpan_test.rs so that following the instructions to the letter on two Imix boards leads to a successful execution of the 6LoWPAN test.

hudson-ayers added some commits Jul 2, 2018

Fix 6lowpan kernel tests to actually work as is in the master branch.…
… This commit updates docs with appropriate commands, correctly inits mac addresses, and fixes erroneous endianness flips
@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 2, 2018

I don't have a good way of evaluating this, but I believe you that it works :) LGTM

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 2, 2018

@ptcrews can you comment on whether the update to decompression of the udp length field looks correct to you, and can you also comment as to why the getters/setters in udp.rs don't convert fields to network order? I remember we had a discussion about converting the UDP fields to network order at some point, and can't remember what the outcome was..

@ptcrews

This comment has been minimized.

Copy link
Contributor

ptcrews commented Jul 3, 2018

@hudson-ayers Hey sorry for the delay - I reviewed the pull request and it looks good. I think the 6LoWPAN UDP support was re-added during the last PR for the networking stack (which is when this bug was introduced), and the fix looks correct to me.

As for the getters/setters changing fields to network-byte order, my last recollection of our discussion was that the UDP header should always store the values in network-byte order, and the getters and setters should be doing the conversion to host-byte order. So the current code is wrong. However, there were some strange issues with the IP addresses and ports, where if we converted them following this convention, Wireshark decoded them incorrectly. I'm not sure if that was due to a bug elsewhere in the networking stack, but I think that was the reason we didn't use the getters/setters for every field.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

@hudson-ayers the build is broken due to a formatting error. Can you fix?

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 5, 2018

@alevy Sure.

FYI I am holding off on merging this while I work on a fix for the checksum errors I was discussing with Paul. Paul is right -- there are some very weird errors with byte ordering, which I think may be indicative of some larger flaws with the design and how we use encode/decode to convert fields to network byte-order.

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 5, 2018

@alevy So this is probably a stupid question...but here goes. I have not used make formatall in the past, as in my past work on the networking stack I was directly making commits to the various branches on Pauls fork of Tock and Paul is the one who ultimately formatted before merging that PR.

However, attempting to run make format from root on this PR results in changes to tons of files unrelated to the files I am changing in this PR (formatting changes occur in like 50 other files scattered all over Tock). This seems wrong to me, given that none of those other changes should have been merged into Tock unless they passed the Travis CI formatting check. So why I am seeing this? The contributing guidelines state that the build system will automatically install and use the correct version of rustfmt, and it seems wrong that all of these files would have been committed to master with formatting errors. Any ideas?

Also, the errors I get when running make format obviously are far more errors than the Travis CI format check was finding (which was just a single error) which seems to imply the formatting checks completed by Travis are different from those done by make formatall

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 5, 2018

@hudson-ayers this is a better question maybe for @ppannuto or @bradjc, but I think that's probably because you're using a different version of rustfmt. I'm not sure how that would happen.

For now, what about just fixing the one formatting error in Travis by hand?

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 5, 2018

One idea is that maybe you have an override for the rust version being used in your tock folder? Is there a suspicious line in ~/.rustup/settings.toml?

Fix byte ordering issues. Now UDP Header fields are stored in network…
… byte order and getters and setters convert to/from host order. This commit also fixes the incorrect UDP checksum implementation and makes some updates to documentation.
@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 6, 2018

@bradjc I see

version = 12

[overrides]

Could that be the culprit?

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 6, 2018

@ptcrews This PR now fixes the checksum issue and some other byte ordering issues. There are still a couple places in sixlowpan_compression.rs that confuse me...Specifically in the decompression of the UDP next header. On lines 766-768, src_port.to_be() and dst_port.to_be() are written into the buffer containing the decompressed packet. However, if you add debug!("src_port.to_be()") it prints the port in little endian, not big endian, implying that prior to this call the ports have had their byte order flipped. I'm not sure exactly what led to this, but the current implementation does work....it just doesn't really make sense.

Regardless of these concerns, I think all of these changes should be applied to the userland branch before it is merged to make sure they do not get lost during the merging process.

@alevy I think this is ready to merge, I just need an approving review to do so.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

Hmm, no that looks fine. What does rustfmt --version say?

@bradjc

bradjc approved these changes Jul 6, 2018

@bradjc bradjc added the P-Upkeep label Jul 6, 2018

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 6, 2018

0.9.0-nightly

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

Ah OK that is too new of a rustfmt version (we are on 0.8.2). How did you install rustc? And what version of rustup do you have rustup --version?

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 6, 2018

Oh, interesting. As far as I remember I just followed the steps in the getting started guide, but I don't really remember, so perhaps I did something different. My rustup version is 1.11.0.

@alevy alevy merged commit 60a9093 into tock:master Jul 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

What should be happening is rustup should be using the rust-toolchain file in the root to select the nightly version to use, and that should have a fixed rustfmt version associated with it (the makefile runs rustup component add rustfmt-preview). It seems like somehow you must be on a newer version of the nightly (which ships with a newer rustfmt).

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jul 6, 2018

@hudson-ayers

Make sure that you haven't configured a workspace override for tock!
Verify it by:

$ rustup override list 

EDIT: oops saw that Brad already had suggested that!

@hudson-ayers

This comment has been minimized.

Copy link
Collaborator

hudson-ayers commented Jul 26, 2018

@bradjc Where can I find the version of rustup that I should have installed? I am on the June 25 nightly for rustc, and my rustup version is 1.11.0. I see where in travis.yml it specifies that the rustc nightly, for which I am on the correct version, but don't see where the rustup version is specified.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 26, 2018

MINIMUM_RUSTUP_VERSION := 1.11.0

We don't have a specified version, but we do require at least 1.11.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment