gnrc_ipv6: only discard invalid source when assigned to interface by miri64 · Pull Request #12442 · RIOT-OS/RIOT

miri64 · 2019-10-14T10:07:44Z

Contribution description

While it is correct to not use an invalid address as a source address, it is incorrect to assume that addresses not assigned to the interface (idx == -1 in the respective piece of code) are invalid: Other than classic forwarding via a FIB, forwarded packets utilizing a IPv6 routing header will pass this check, like any other packet sent by this node. The source address for these is not on the given node, so e.g. source routing is not possible at the moment.

Testing procedure

Testing procedures from #11970 should still work (was unable to test this atm due to lacking hardware).

With #12440 merged tests/gnrc_rpl_srh passes (requires dist/tools/tapsetup/tapsetup to be called before and also must be called with sudo).

Issues/PRs references

Follow-up to #11970
Fixes parts of #12436
Accompanies #12440

While it is correct to not use an invalid address as a source address, it is incorrect to assume that addresses not assigned to the interface (`idx == -1` in the respective piece of code) are invalid: Other than classic forwarding via a FIB, forwarded packets utilizing a IPv6 routing header will pass this check, like any other packet sent by this node. The source address for these is not on the given node, so e.g. source routing is not possible at the moment.

miri64 · 2019-10-14T13:43:34Z

Rebased to current master to include now merged #12440.

kb2ma · 2019-10-14T17:05:04Z

At this point, tests/gnrc_rpl_srh should work, correct? Instead b5b52c7 results like below for me:

Traceback (most recent call last):
  File "/home/kbee/dev/riot/repo/tests/gnrc_rpl_srh/tests/01-run.py", line 384, in <module>
    sys.exit(run(testfunc, timeout=1, echo=False))
  File "/home/kbee/dev/riot/repo/dist/pythonlibs/testrunner/__init__.py", line 29, in run
    testfunc(child)
  File "/home/kbee/dev/riot/repo/tests/gnrc_rpl_srh/tests/01-run.py", line 368, in testfunc
    run(test_forward_uncomp)
  File "/home/kbee/dev/riot/repo/tests/gnrc_rpl_srh/tests/01-run.py", line 361, in run
    raise e
  File "/home/kbee/dev/riot/repo/tests/gnrc_rpl_srh/tests/01-run.py", line 357, in run
    func(child, tap, hwaddr_dst, lladdr_dst, lladdr_src)
  File "/home/kbee/dev/riot/repo/tests/gnrc_rpl_srh/tests/01-run.py", line 265, in test_forward_uncomp
    assert(IPv6ExtHdrRouting in p)
AssertionError

cgundogan · 2019-10-14T18:01:11Z

Interestingly, 7 out of 10 times I get a SUCCESS. However, the other 3 times I see the same error described by @kb2ma

cgundogan · 2019-10-14T18:10:35Z

Sometimes, I even hit another assert:

Traceback (most recent call last):
  File "/tmp/RIOT/tests/gnrc_rpl_srh/tests/01-run.py", line 384, in <module>
    sys.exit(run(testfunc, timeout=1, echo=False))
  File "/tmp/RIOT/dist/pythonlibs/testrunner/__init__.py", line 29, in run
    testfunc(child)
  File "/tmp/RIOT/tests/gnrc_rpl_srh/tests/01-run.py", line 366, in testfunc
    run(test_multicast_addr)
  File "/tmp/RIOT/tests/gnrc_rpl_srh/tests/01-run.py", line 361, in run
    raise e
  File "/tmp/RIOT/tests/gnrc_rpl_srh/tests/01-run.py", line 357, in run
    func(child, tap, hwaddr_dst, lladdr_dst, lladdr_src)
  File "/tmp/RIOT/tests/gnrc_rpl_srh/tests/01-run.py", line 227, in test_multicast_addr
    assert(len(p) == 0)
AssertionError

miri64 · 2019-10-15T07:12:03Z

Ok, then it is still flakey. I will have another look if it is related or yet another regression introduced since last release.

miri64 · 2019-10-15T07:19:09Z

Ok, a quick sniff with Wireshark during a test run confirmed to me, that this is the same sniffer syncing issue I already described in the testing procedures of #10392 (the packets are forwarded as they should, but they are not picked up by the scapy sniffer). So this is a bug in the test, not in the network stack. If I find the time, I will port these tests to scapy automata, similar as I did for emcute in #11823, but for this release I think we can go as is.

miri64 · 2019-10-15T07:20:30Z

For the other observation by @cgundogan, I think it picks up a packet previously on the medium, so a syncing issue again.

miri64 · 2019-10-15T07:21:17Z

(I don't see any ICMPv6 error messages nor second UDP packet on the TAP interface during the run of those specific tests, even when they fail)

cgundogan

The fix is sensible and tests/gnrc_rpl_srh runs successful (excluding the sporadic failures due to scapy sniffer running out-of-sync)

miri64 · 2019-10-15T09:19:55Z

Testing procedures from #11970 should still work (was unable to test this atm due to lacking hardware).

Has someone retested this as well?

cgundogan · 2019-10-15T09:29:21Z

From #11970:

Classic ND should work as expected. To test this I used radvd on my host system with the following config

interface tapbr0 {
    AdvSendAdvert on;
    prefix 2001:db8:0:1::/64
    {
         AdvOnLink on;
         AdvAutonomous on;
    };
};

If using native (with dist/tools/tapsetup/tapsetup to create the TAP interfaces) the 2001:db8:0:1:: address of the node should become valid a few seconds after the node started.

The address is still set to VAL on native for me.

I try to get my hands on a pi for the 6lo-nd test

cgundogan · 2019-10-15T11:46:26Z

I confirm now that this PR does not change the following behaviour:

RasPi BR announces ABROs. RIOT node configures a global address with TNT[3] set. Ping does not work.
Pinging in above setup works, when compiling with GNRC_IPV6_NIB_CONF_SLAAC=1

miri64 · 2019-10-15T11:53:41Z

Thanks for the review and testing!

miri64 · 2019-10-15T11:56:32Z

Backport provided in #12458

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Oct 14, 2019

miri64 added this to the Release 2019.10 milestone Oct 14, 2019

miri64 requested review from MrKevinWeiss, kb2ma and smlng October 14, 2019 10:07

This was referenced Oct 14, 2019

tests/gnrc_rpl_srh failures #12436

Closed

tests/gnrc_rpl_srh: fix test assumption #12440

Merged

tests/gnrc_rpl_srh: fix test assumption [backport 2019.10] #12443

Merged

miri64 force-pushed the gnrc_ipv6/fix/only-local-invalid-src branch from 95a6ea9 to b5b52c7 Compare October 14, 2019 13:43

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 14, 2019

cgundogan approved these changes Oct 15, 2019

View reviewed changes

cgundogan merged commit 626f68c into RIOT-OS:master Oct 15, 2019

miri64 deleted the gnrc_ipv6/fix/only-local-invalid-src branch October 15, 2019 11:53

miri64 mentioned this pull request Oct 15, 2019

gnrc_ipv6: only discard invalid source when assigned to interface [backport 2019.10] #12458

Merged

kb2ma mentioned this pull request Oct 15, 2019

Release 2019.10 - RC1 RIOT-OS/Release-Specs#140

Closed

80 tasks

Conversation

miri64 commented Oct 14, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Oct 14, 2019

Uh oh!

kb2ma commented Oct 14, 2019

Uh oh!

cgundogan commented Oct 14, 2019

Uh oh!

cgundogan commented Oct 14, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

cgundogan commented Oct 15, 2019

Uh oh!

cgundogan commented Oct 15, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

miri64 commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants