gnrc_ipv6: move ipv6_ext iteration out of ext_demux() by miri64 · Pull Request #10234 · RIOT-OS/RIOT

miri64 · 2018-10-23T22:42:29Z

Contribution description

Since with #10233 we now assume IPv6 packets always to not be pre-parsed, we can iterate over the extension headers by gradually "eating" them away. This allows us to move the iteration over them out of gnrc_ipv6_ext_demux() and into gnrc_ipv6_demux().

By moving the iteration over all extension headers out of gnrc_ipv6_ext_demux() we also can

simplify the extension header handling a lot, as it now just a loop inside gnrc_ipv6_demux(),
remove the recursion to gnrc_ipv6_demux() within gnrc_ipv6_ext_demux().
some cleanup, possible due to all the things above.

I also piggy-backed some follow-up work that are related to demuxing and the fact that they are not pre-parsed:

since they are not preparsed, we don't haved to specifically write-protect all the extension headers up to the IPv6 packet, as the whole (un-parsed) packet was already write protected when first received.
since the gnrc_ipv6_ext doesn't recurse into gnrc_ipv6_demux() anymore, that function can be made private again.

Testing procedure

I used scapy to inject packets with and without extension headers into a native instance of gnrc_networking:

On RIOT:

udp server start 1337

Within scapy (be aware that to send Ethernet frames, root privileges are required, so best start it with sudo scapy):

DST_ADDR = "fe80::ac4e:aeff:fec0:caab" # use your RIOT node's address here instead
# first send a simple UDP packet to test if the server is working
sendp(Ether() / 
      IPv6(dst=DST_ADDR) /
      UDP(sport=1339, dport=1337) /
      "\x00\x01", iface="tapbr0")
# now send some extension headers (we assume they all are not parsed)
sendp(Ether() /
      IPv6(dst=DST_ADDR) /
      IPv6ExtHdrDestOpt() /
      UDP(sport=1339, dport=1337) /
      "\x00\x01", iface="tapbr0")
sendp(Ether() /
      IPv6(dst=DST_ADDR) /
      IPv6ExtHdrRouting() /
      UDP(sport=1339, dport=1337) /
      "\x00\x01", iface="tapbr0")
# ...

You can find some documentation on IPv6 extension headers in this document

You can also just redo the testing procedures in #10229.

Issues/PRs references

Depends on #10233 (and its dependencies)

…HEAD

… HEAD

…HEAD

miri64 · 2018-10-25T17:54:08Z

Rebased to current #10233

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c

sys/include/net/gnrc/pktbuf.h

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c

miri64 · 2018-10-29T19:32:07Z

Rebased to current #10233.

miri64 · 2018-10-30T17:06:57Z

Already found the first batch of errors due to my tests :-)

miri64 · 2018-12-13T13:30:50Z

Fixed typo in as squashable commit.

jia200x · 2018-12-13T13:39:12Z

1-fundamentals done. Nice cleanup btw!

jia200x · 2018-12-13T14:56:49Z

well, it seems to work. I will run #10229 tests as well

jia200x · 2018-12-13T15:06:21Z

#10229 tests run fine.

miri64 · 2018-12-13T15:19:52Z

May I squash?

jia200x · 2018-12-13T15:30:18Z

May I squash?

Yes, sure

jia200x · 2018-12-13T15:31:24Z

vera++ doesn't report problems with the introduced changes. There are some consecutive empty lines but not in the scope of this PR.

miri64 · 2018-12-13T15:33:14Z

Squashed

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

Since with RIOT-OS#10233 we now assume IPv6 packets always to not be pre-parsed, we can iterate over the extension headers by gradually "eating" them away. This allows us to move the iteration over them out of `gnrc_ipv6_ext_demux()` and into `gnrc_ipv6_demux()`. By moving the iteration over all extension headers out of `gnrc_ipv6_ext_demux()` we also can 1. simplify the extension header handling a lot, as it now just a loop inside `gnrc_ipv6_demux()`, 2. remove the recursion to `gnrc_ipv6_demux()` within `gnrc_ipv6_ext_demux()`.

As `pkt` isn't pre-parsed the write-protection of *the whole* packet (except the netif-header) comes for free, when this was done in the receive routine of IPv6.

Since the recursion into `gnrc_ipv6_demux()` was removed in `gnrc_ipv6_ext`, `gnrc_ipv6.c` is the only user of this function, so it can be made private. It was only made public so it can be used from `gnrc_ipv6_ext`.

With the preceding changes the subject of the deprecation note on `gnrc_pktbuf_duplicate_upto()` becomes actual and thus doesn't need to be referred to in future but past tense.

miri64 · 2018-12-13T16:54:11Z

Murdock likes it now :)

jia200x

ACK

jia200x · 2018-12-14T09:43:55Z

Thanks @miri64 !

miri64 · 2018-12-14T10:12:08Z

Thanks for your review!

miri64 added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 23, 2018

miri64 requested review from bergzand, cgundogan and jnohlgard October 23, 2018 22:42

miri64 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Oct 23, 2018

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from 8a4ff3e to b766b10 Compare October 23, 2018 22:58

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_ipv6/enh/iterate-out-of-ext' (RIOT-OS#10234) into …

d2247ab

…HEAD

miri64 mentioned this pull request Oct 24, 2018

gnrc_ipv6_ext: needs heavy refactoring #10133

Closed

5 tasks

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018

Merge branch 'gnrc_ipv6/enh/iterate-out-of-ext' (RIOT-OS#10234) into…

80fd345

… HEAD

miri64 mentioned this pull request Oct 24, 2018

gnrc_ipv6_ext: merge _handle_rh and gnrc_ipv6_ext_rh_process #10238

Merged

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from 870a055 to 252540e Compare October 24, 2018 12:56

This was referenced Oct 24, 2018

gnrc_ipv6_ext_rh: add ICMPv6 error message sending #10242

Merged

gnrc_ipv6: handle hop-by-hop option before forwarding #10244

Merged

gnrc_ipv6_ext: move ipv6_ext_rh (partly) to GNRC #10231

Merged

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch 2 times, most recently from 92f18bb to 8029b4f Compare October 24, 2018 19:33

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018

Merge branch 'gnrc_ipv6/enh/iterate-out-of-ext' (RIOT-OS#10234) into …

e80d801

…HEAD

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from 8029b4f to 90c8cea Compare October 25, 2018 17:53

Lotterleben reviewed Oct 25, 2018

View reviewed changes

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c Outdated Show resolved Hide resolved

Lotterleben reviewed Oct 25, 2018

View reviewed changes

sys/include/net/gnrc/pktbuf.h Show resolved Hide resolved

Lotterleben reviewed Oct 25, 2018

View reviewed changes

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c Outdated Show resolved Hide resolved

RIOT-OS deleted a comment Oct 26, 2018

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from 82308cd to b1f1b81 Compare October 29, 2018 19:31

RIOT-OS deleted a comment Oct 30, 2018

RIOT-OS deleted a comment Oct 31, 2018

jia200x added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Dec 13, 2018

jia200x added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Dec 13, 2018

jia200x added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 13, 2018

jia200x added the Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines label Dec 13, 2018

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from 76256a9 to d7c4078 Compare December 13, 2018 15:33

jia200x added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Dec 13, 2018

jia200x requested changes Dec 13, 2018

View reviewed changes

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c Show resolved Hide resolved

miri64 and others added 6 commits December 13, 2018 17:22

gnrc_ipv6_ext: remove unnecessary pkt write-protection

f671a87

As `pkt` isn't pre-parsed the write-protection of *the whole* packet (except the netif-header) comes for free, when this was done in the receive routine of IPv6.

gnrc_ipv6: make next-header demuxer private

764ed8c

Since the recursion into `gnrc_ipv6_demux()` was removed in `gnrc_ipv6_ext`, `gnrc_ipv6.c` is the only user of this function, so it can be made private. It was only made public so it can be used from `gnrc_ipv6_ext`.

gnrc_ipv6: clean-up unrequired stuff after demux rework

e6df40d

gnrc_ipv6: rename should_release for clarity

b8f71c3

gnrc_pktbuf: update tense of deprecation note ;-)

4257b70

With the preceding changes the subject of the deprecation note on `gnrc_pktbuf_duplicate_upto()` becomes actual and thus doesn't need to be referred to in future but past tense.

miri64 force-pushed the gnrc_ipv6/enh/iterate-out-of-ext branch from d7c4078 to 4257b70 Compare December 13, 2018 16:22

jia200x approved these changes Dec 14, 2018

View reviewed changes

jia200x merged commit b709e63 into RIOT-OS:master Dec 14, 2018

miri64 deleted the gnrc_ipv6/enh/iterate-out-of-ext branch December 14, 2018 10:11

aabadie added this to the Release 2019.01 milestone Dec 19, 2018

miri64 mentioned this pull request Aug 10, 2019

gnrc_ipv6_ext: conceptual error in packet buffer usage #7185

Closed

Conversation

miri64 commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Oct 29, 2018

Uh oh!

miri64 commented Oct 30, 2018

Uh oh!

miri64 commented Dec 13, 2018

Uh oh!

jia200x commented Dec 13, 2018

Uh oh!

jia200x commented Dec 13, 2018

Uh oh!

jia200x commented Dec 13, 2018

Uh oh!

miri64 commented Dec 13, 2018

Uh oh!

jia200x commented Dec 13, 2018

Uh oh!

jia200x commented Dec 13, 2018

Uh oh!

miri64 commented Dec 13, 2018

Uh oh!

Uh oh!

miri64 commented Dec 13, 2018

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Dec 14, 2018

Uh oh!

miri64 commented Dec 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miri64 commented Oct 23, 2018 •

edited

Loading