gnrc_ipv6_ext: use send for forwarding with RH by miri64 · Pull Request #10229 · RIOT-OS/RIOT

miri64 · 2018-10-23T16:30:13Z

Contribution description

Though this change might seem more complicated, it has the benefit, that
after #9484 we don't have to assume that a received packet within IPv6's
receive function can be handed to the function pre-parsed, making that
function far less complicated (will be provided in a future PR).

Also this might give the forwarding via routing header a little
performance boost, as we now don't receive the packet first only to
forward it later-on.

Testing procedure

From #10229 (comment) (corrected one inconsistency though):

Compile examples/gnrc_networking with USEMODULE += gnrc_rpl_srh for native

Start two native instances connected via a bridge tapbr0
native instance 1 (native1):
ifconfig 6 add 2001:db8::1
udp server start 1337
native instance 2 (native2):
udp server start 1337
Scapy:
DST_HWADDR="<ethernet address of native2>"
SRC_GLB="<global address of tapbr0>"
DST_GLB="<global address of native2>"
sendp(Ether(dst=DST_HWADDR) /
      IPv6(src=SRC_GLB, dst=DST_GLB) /
      IPv6ExtHdrRouting(type=3, segleft=0) /
      UDP(dport=1337) / "\x01", iface="tapbr0")
sendp(Ether(dst=DST_HWADDR) /
      IPv6(src=SRC_GLB, dst=DST_GLB) /
      IPv6ExtHdrRouting(type=3, segleft=1, addresses=["2001:db8::1"]) /
      UDP(dport=1337) / "\x02", iface="tapbr0")
First packet should be outputted on native instance 1 (can be identified by the 01 payload).

Second packet should be outputted on native instance 2 (can be identified by 02 payload).

Both should contain a NETTYPE_UNKNOWN snip (or NETTYPE_IPV6_EXT when #10383 was merged) containing the extension header.

Issues/PRs references

Depends on ~~#10226~~ (merged). Needs ~~#10422 for testing~~ (merged)

…to HEAD

miri64 · 2018-10-25T17:25:44Z

Rebased to current #10226 and master.

miri64 · 2018-10-25T21:53:13Z

Rebased to current master and ready for review!

miri64 · 2018-11-06T15:53:08Z

@bergzand do you want to have a look?

bergzand

Initial review

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

bergzand · 2018-11-07T09:06:30Z

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

+    /* remove L2 headers around IPV6 */
+    netif_snip = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if (netif_snip != NULL) {
+        gnrc_pktbuf_remove_snip(pkt, netif_snip);


Shouldn't this be something like pkt = gnrc_pktbuf_remove_snip(pkt, netif_snip);?

Technically you are right, but since it is highly unlikely, that the netif_snip is at the start of the list at this point, it isn't necessary. For consistency sake I will however do it.

bergzand · 2018-11-07T09:07:28Z

@miri64 Do you happen to have a bit of python/scapy code I can use to test this?

miri64 · 2018-11-07T09:34:48Z

@bergzand This branch is going to provide the new tests but at the moment it is a very moving target (as I force-push and squash to it). For compile tests, I just used the current version of tests/gnrc_ipv6_ext and for functionality tests I used gnrc_networking with USEMODULE += "gnrc_rpl_srh" and induced something like

sendp(Ether() /
      IPv6(dst=DST_ADDR) /
      IPv6ExtHdrRouting(type=3, segleft=2, addresses=["fd02::1", "fd02::2"]) /
      UDP(sport=1339, dport=1337) /
      "\x00\x01", iface="tapbr0")

I'm also planning to provide some RPLRoutingHdr class on the testing branch, so the construction of compressed addresses isn't all that tricky ;-).

miri64 · 2018-11-07T09:39:23Z

Adressed comments

…to HEAD

bergzand · 2018-11-16T20:42:14Z

I've played around with the branch here, but I'm not sure what functionality to expect with the runtime tests. I can bounce a frame from one instance to another, but proper reception on the destination host seems to fail.

bergzand · 2018-11-16T20:43:30Z

but proper reception on the destination host seems to fail.

Meaning that after starting a UDP server on the destination host, I don't seem to be able to craft a frame that actually arrives at the UDP handler thread on that host.

miri64 · 2018-11-17T01:10:08Z

I'll provide some test descriptions tomorrow (though #10392 provides a test case for this)

miri64 · 2018-11-17T12:03:59Z

Ok here is a test descripition. However, you will need #10422 for it to work properly:

Compile examples/gnrc_networking with USEMODULE += gnrc_rpl_srh for native
Start two native instances connected via a bridge

native instance 1:

ifconfig 6 add 2001:db8::1
udp server start 1337

native instance 2:
```
udp server start 1337
```

Scapy:

sendp(Ether(dst=DST_HWADDR) /
      IPv6(src=SRC_GLB, dst=DST_GLB) /
      IPv6ExtHdrRouting(type=3, segleft=0, addresses=["2001:db8::1"]) /
      UDP(dport=1337) / "\x01", iface="tapbr0")
sendp(Ether(dst=DST_HWADDR) /
      IPv6(src=SRC_GLB, dst=DST_GLB) /
      IPv6ExtHdrRouting(type=3, segleft=1, addresses=["2001:db8::1"]) /
      UDP(dport=1337) / "\x02", iface="tapbr0")

First packet should be outputted on native instance 1 (can be identified by the 01 payload).

Second packet should be outputted on native instance 2 (can be identified by 02 payload).

Both should contain a NETTYPE_UNKNOWN snip (or NETTYPE_IPV6_EXT when #10383 was merged) containing the extension header.

miri64 · 2018-11-17T13:57:39Z

Shall I wait with the rebase until #10419 is fixed or immediately when #10422 is merged?

bergzand · 2018-11-17T14:00:18Z

Shall I wait with the rebase until #10419 is fixed or immediately when #10422 is merged?

Reviewing #10421 at the moment, feel free to rebase immediately, but I don't expect #10421 to take long to review

miri64 · 2018-11-17T14:00:49Z

Then I wait for #10421 to be merged first :-)

miri64 · 2018-12-03T13:35:14Z

@gebart could you maybe have a look? You ACK'd the dependency #10226 and Koen mentioned to me off-channel, that he currently doesn't have the time to review. Or maybe @jia200x? You already gathered experience testing RH handling in #10227 ;-).

@bergzand

Comments were addressed, but @bergzand currently doesn't have time to review.

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

jia200x · 2018-12-06T13:05:35Z

Followed the testing procedure. Here are the results:

native1:

> PKTDUMP: data received:
~~ SNIP  0 - size:   1 byte, type: NETTYPE_UNDEF (0)
00000000  02
~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
   src-port:    53  dst-port:  1337
   length: 9  cksum: 0xd2a0
~~ SNIP  2 - size:  24 byte, type: NETTYPE_UNKNOWN (2)
00000000  11  02  03  00  00  00  00  00  20  01  0D  B8  00  00  00  00
00000010  68  56  A8  FF  FE  34  91  E8
~~ SNIP  3 - size:  40 byte, type: NETTYPE_IPV6 (1)
traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
flow label: 0x00000
length: 33  next header: 43  hop limit: 63
source address: 2001:db8::4c3a:39ff:fe4f:45d1
destination address: 2001:db8::1
~~ SNIP  4 - size:  20 byte, type: NETTYPE_NETIF (-1)
if_pid: 6  rssi: 0  lqi: 0
flags: 0x0
src_l2addr: 6A:56:A8:34:91:E8
dst_l2addr: 4E:3A:39:4F:45:D2
~~ PKT    -  5 snips, total size:  93 byte

native2

> PKTDUMP: data received:
~~ SNIP  0 - size:   1 byte, type: NETTYPE_UNDEF (0)
00000000  01
~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
   src-port:    53  dst-port:  1337
   length: 9  cksum: 0x322e
~~ SNIP  2 - size:   8 byte, type: NETTYPE_UNKNOWN (2)
00000000  11  00  03  00  00  00  00  00
~~ SNIP  3 - size:  40 byte, type: NETTYPE_IPV6 (1)
traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
flow label: 0x00000
length: 17  next header: 43  hop limit: 64
source address: 2001:db8::4c3a:39ff:fe4f:45d1
destination address: 2001:db8::6856:a8ff:fe34:91e8
~~ SNIP  4 - size:  20 byte, type: NETTYPE_NETIF (-1)
if_pid: 6  rssi: 0  lqi: 0
flags: 0x0
src_l2addr: 4E:3A:39:4F:45:D1
dst_l2addr: 6A:56:A8:34:91:E8
~~ PKT    -  5 snips, total size:  77 byte

Got same results as described in the testing procedure

jia200x · 2018-12-06T13:09:50Z

code style (vera++) is OK

jia200x · 2018-12-06T13:13:00Z

code style (vera++) is OK

BTW the file has 2 empty lines in the end. But it's not related to this PR ;)

miri64 · 2018-12-06T13:36:03Z

May I squash?

jia200x · 2018-12-06T13:46:27Z

May I squash?

Yes, please

Though this change might seem more complicated, it has the benefit, that after RIOT-OS#9484 we don't have to assume that a received packet within IPv6's receive function can be handed to the function pre-parsed, making that function far less complicated (will be provided in a future PR). Also this might give the forwarding via routing header a little performance boost, as we now don't *receive* the packet first only to forward it later-on.

miri64 · 2018-12-06T13:48:30Z

Squashed

jia200x

ACK

…/shortcut-forward

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 23, 2018

miri64 requested a review from bergzand October 23, 2018 16:30

miri64 changed the title ~~Gnrc ipv6 ext/enh/shortcut forward~~ gnrc_ipv6_ext: use send for forwarding with RH Oct 23, 2018

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

Merge branch 'gnrc_ipv6_ext/enh/shortcut-forward' (RIOT-OS#10229) in…

de9889f

…to HEAD

miri64 mentioned this pull request Oct 23, 2018

gnrc_ipv6: assume no preparsed packets #10233

Merged

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

Merge branch 'gnrc_ipv6_ext/enh/shortcut-forward' (RIOT-OS#10229) in…

626d8b2

…to HEAD

miri64 force-pushed the gnrc_ipv6_ext/enh/shortcut-forward branch from 0365e20 to 1fa21d6 Compare October 25, 2018 17:25

miri64 force-pushed the gnrc_ipv6_ext/enh/shortcut-forward branch from 1fa21d6 to b505327 Compare October 25, 2018 21:52

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 25, 2018

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 25, 2018

bergzand previously requested changes Nov 7, 2018

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Nov 8, 2018

Merge branch 'gnrc_ipv6_ext/enh/shortcut-forward' (RIOT-OS#10229) in…

1eb2598

…to HEAD

miri64 mentioned this pull request Nov 17, 2018

gnrc_ipv6: fix _fill_ipv6_hdr() for pure IPv6 packets #10422

Merged

jia200x added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Dec 4, 2018

jia200x reviewed Dec 4, 2018

View reviewed changes

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

jia200x added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines and removed Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Dec 4, 2018

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

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 6, 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 6, 2018

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

miri64 force-pushed the gnrc_ipv6_ext/enh/shortcut-forward branch from a710a29 to b530c1b Compare December 6, 2018 13:48

jia200x approved these changes Dec 6, 2018

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 6, 2018

miri64 merged commit 9bb458d into RIOT-OS:master Dec 6, 2018

miri64 deleted the gnrc_ipv6_ext/enh/shortcut-forward branch December 6, 2018 15:18

miri64 mentioned this pull request Dec 11, 2018

gnrc_ipv6: move ipv6_ext iteration out of ext_demux() #10234

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 11, 2018

fixup! Merge pull request RIOT-OS#10229 from miri64/gnrc_ipv6_ext/enh…

c263167

…/shortcut-forward

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

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!

miri64 commented Oct 25, 2018

Uh oh!

miri64 commented Nov 6, 2018

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bergzand Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

miri64 Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

bergzand commented Nov 7, 2018

Uh oh!

miri64 commented Nov 7, 2018

Uh oh!

miri64 commented Nov 7, 2018

Uh oh!

bergzand commented Nov 16, 2018

Uh oh!

bergzand commented Nov 16, 2018

Uh oh!

miri64 commented Nov 17, 2018

Uh oh!

miri64 commented Nov 17, 2018

Uh oh!

miri64 commented Nov 17, 2018

Uh oh!

bergzand commented Nov 17, 2018

Uh oh!

miri64 commented Nov 17, 2018

Uh oh!

miri64 commented Dec 3, 2018

Uh oh!

Uh oh!

jia200x commented Dec 6, 2018

Uh oh!

jia200x commented Dec 6, 2018

Uh oh!

jia200x commented Dec 6, 2018

Uh oh!

miri64 commented Dec 6, 2018

Uh oh!

jia200x commented Dec 6, 2018

Uh oh!

miri64 commented Dec 6, 2018

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

miri64 commented Oct 23, 2018 •

edited

Loading