gnrc_ipv6: handle hop-by-hop option before forwarding by miri64 · Pull Request #10244 · RIOT-OS/RIOT

miri64 · 2018-10-24T16:37:30Z

Contribution description

Hop-by-hop options need to be handled separately, as they are required to be handled hop-by-hop, as the name implies. Now after weeks of rework, we are now finally able to do this cleanly within gnrc_ipv6.

Testing procedure

Since there is no proper hop-by-hop option support yet, I repeated my tests from #10234, but also tested with duplicate Hop-by-hop header, as this is an error case now, as per RFC 8200 and activated DEBUG output to verify that it is indeed handled as an error.

Issues/PRs references

Depends on ~~#8594~~ (merged) and ~~#10234 (and its dependencies)~~ (merged).

miri64 · 2018-10-29T19:48:56Z

Rebased to current #10234.

miri64 · 2018-10-30T17:07:26Z

Already found some errors due to my tests :-)

miri64 · 2018-11-08T17:45:36Z

Rebased to current #10234.

miri64 · 2018-11-14T16:17:14Z

Rebased to current master and dependencies (and added #8594 as a dependency and updated accordingly).

miri64 · 2018-11-17T15:40:24Z

Rebased to current #10244.

miri64 · 2018-12-14T10:35:20Z

Rebased to current master. No longer dependent on other PRs.

miri64 · 2018-12-14T10:37:18Z

(also squashed)

jia200x · 2018-12-14T12:16:06Z

sys/include/net/gnrc/ipv6/ext.h

+                                          uint8_t *nh);
+#else   /* defined(MODULE_GNRC_IPV6_EXT) || defined(DOXYGEN) */
+/* NOPs to make the usage code more readable */
+#define gnrc_ipv6_ext_process_all(pkt, first_nh)    (pkt);


This is a little bit misleading IMO. This macro looks like a function and someone could spend some time looking to the actual implementation of gnrc_ipv6_ext_process_all instead of the NOP.

Also I would prefer to move this macro to the C file and just leave only the prototype here.

I would do something like:

[gnrc_ipv6_ext.c]

#ifdef MODULE_GNRC_IPV6_EXT #define GNRC_IPV6_EXT_PROCESS_ALL(pkt, first_nh) gnrc_ipv6_ext_process_all(pkt, nh) #else #define GNRC_IPV6_EXT_PROCESS_ALL(pkt, first_nh) (pkt) #endif

That way, it's clear GNRC_IPV6_EXT_PROCESS_ALL is a macro

This is non-sensical. Then the user needs to use ifdefs anyway. The point of these NOP-macros is so you can use the functions in your code without worrying that the module is included or not.

I don't like the uppercase wrapper-solution either ;)

Then the user needs to use ifdefs anyway.

But at the same time the user shouldn't use gnrc_ipv6_ext_process_all if the module is not included. I see this only as a helper for gnrc_ipv6_ext.c

(I won't block the PR for this but still prefer helpers rather than NOPs :)

To be more accurate, I meant I'm not convinced with mixing a macro and a function with the same name. I would prefer just use a second macro (as I described) or a weak-like symbol mechanism.

Or at least add a tiny note before the usage of this function indicating it resolves to pkt if MODULE_GNRC_IPV6_EXT is not included :)

This mechanism is already used through-out the code and the behavior is the same as if gnrc_ipv6_ext would be in (but since the stack doesn't know about extension headers it is not handling them).

ok, let's get it in

Some examples:

RIOT/sys/include/net/gnrc/icmpv6/error.h

Lines 95 to 102 in b709e63

#define gnrc_icmpv6_error_dst_unr_send(code, orig_pkt) \

(void)code; (void)orig_pkt

#define gnrc_icmpv6_error_pkt_too_big_send(mtu, orig_pkt) \

(void)mtu; (void)orig_pkt

#define gnrc_icmpv6_error_time_exc_send(code, orig_pkt) \

(void)code; (void)orig_pkt

#define gnrc_icmpv6_error_param_prob_send(code, ptr, orig_pkt) \

(void)code; (void)ptr, (void)orig_pkt

RIOT/sys/include/net/gnrc/netif/internal.h

Lines 470 to 471 in b709e63

#define gnrc_netif_ipv6_iid_from_addr(netif, addr, addr_len, iid) (-ENOTSUP)

#define gnrc_netif_ipv6_iid_to_addr(netif, iid, addr) (-ENOTSUP)

The ironic thing: Some reviewers specifically asked me to do it this way...

yes, I see. There are several ways to handle stubs. I'm fine ;)

jia200x · 2018-12-14T13:29:33Z

I'm ok with 1-fundamentals and 2-code-design. I will test now

jia200x · 2018-12-14T15:50:47Z

Sending one HopByHop header:

>>> sendp(Ether() / 
...:       IPv6(dst=DST_ADDR) / 
...:       IPv6ExtHdrHopByHop() / 
...:       UDP(sport=1339, dport=1337) / 
...:       "\x00\x01", iface="tapbr0")

I receive:

Success: started UDP server on port 1337
> ipv6: GNRC_NETAPI_MSG_TYPE_RCV received
ipv6: Received (src = fe80::a9f1:fcc1:4389:4748, dst = fe80::4c3a:39ff:fe4f:45d2, next header = 0, length = 18)
ipv6_ext: next header = 0ipv6_ext: skipping over unsupported extension header
ipv6: forward nh = 17 to other threads
ipv6: waiting for incoming message.
PKTDUMP: data received:
~~ SNIP  0 - size:   2 byte, type: NETTYPE_UNDEF (0)
00000000  00  01
~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
   src-port:  1339  dst-port:  1337
   length: 10  cksum: 0xfc82
~~ SNIP  2 - size:   8 byte, type: NETTYPE_UNKNOWN (2)
00000000  11  00  01  04  00  00  00  00
~~ SNIP  3 - size:  40 byte, type: NETTYPE_IPV6 (1)
traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
flow label: 0x00000
length: 18  next header: 0  hop limit: 64
source address: fe80::a9f1:fcc1:4389:4748
destination address: fe80::4c3a:39ff:fe4f:45d2
~~ SNIP  4 - size:  20 byte, type: NETTYPE_NETIF (-1)
if_pid: 6  rssi: 0  lqi: 0
flags: 0x0
src_l2addr: 00:50:B6:8C:B5:1A
dst_l2addr: FF:FF:FF:FF:FF:FF
~~ PKT    -  5 snips, total size:  78 byte

When sending a malformed packet:

>>> sendp(Ether() / 
...:       IPv6(dst=DST_ADDR) / 
...:       IPv6ExtHdrHopByHop() / 
...:       IPv6ExtHdrHopByHop() / 
...:       UDP(sport=1339, dport=1337) / 
...:       "\x00\x01", iface="tapbr0")                                                                                                                        
WARNING: Mac address to reach destination

I get

> ipv6: GNRC_NETAPI_MSG_TYPE_RCV received
ipv6: Received (src = fe80::a9f1:fcc1:4389:4748, dst = fe80::4c3a:39ff:fe4f:45d2, next header = 0, length = 26)
ipv6_ext: next header = 0ipv6_ext: skipping over unsupported extension header
ipv6: duplicate Hop-by-Hop header
ipv6: packet's extension header was errorneous or packet was consumed due to it
ipv6: waiting for incoming message.

As expected

jia200x · 2018-12-14T15:56:53Z

nothing interesting to report code-style-wise

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

They are handled separately anyway and this allows us to handle the Hop-by-hop option *before* forwarding in a later step.

This way e.g. a raw socket listening for an extension headers protocol number also get's it.

The function is now only called by `gnrc_ipv6_ext_process_hopopt()` and `gnrc_ipv6_ext_process_all()`, both are part of the `gnrc_ipv6_ext` module.

miri64

Fixed comments and squashed immediately. I also added a fix for a broken debug message I noticed in your dump above (that is already present in master, but no harm piggy-backing it here, right)

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

miri64 · 2018-12-14T18:12:48Z

(that is already present in master, but no harm piggy-backing it here, right)

(Well it does if Git resolves the conflict automatically but wrong. Fixed that)

jia200x · 2018-12-17T09:22:00Z

ACK. Thanks!

miri64 · 2018-12-17T09:25:55Z

Thank you for the review and thorough testing :-)

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Oct 24, 2018

miri64 requested review from BytesGalore, bergzand and cgundogan October 24, 2018 16:37

This was referenced Oct 24, 2018

ipv6/rpl: add hop-by-hop extension handling #7231

Draft

gnrc_ipv6_ext: needs heavy refactoring #10133

Closed

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch 3 times, most recently from 1fc934a to 5d076df Compare October 25, 2018 18:06

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

RIOT-OS deleted a comment Oct 26, 2018

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 5d076df to 8028f6e Compare October 29, 2018 19:48

RIOT-OS deleted a comment Oct 30, 2018

RIOT-OS deleted a comment Oct 31, 2018

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 486c053 to 92369f2 Compare November 8, 2018 17:45

RIOT-OS deleted a comment Nov 9, 2018

miri64 mentioned this pull request Nov 13, 2018

tests/gnrc_ipv6_ext: port to scapy #10382

Merged

3 tasks

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 92369f2 to 6b5b714 Compare November 14, 2018 16:17

miri64 mentioned this pull request Nov 14, 2018

tests: introduce dedicated gnrc_rpl_srh test #10392

Merged

RIOT-OS deleted a comment Nov 15, 2018

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 6b5b714 to 84a6e5a Compare November 17, 2018 15:37

RIOT-OS deleted a comment Nov 17, 2018

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 84a6e5a to 7140adc Compare November 18, 2018 18:49

RIOT-OS deleted a comment Nov 18, 2018

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

jia200x requested changes Dec 14, 2018

View reviewed changes

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 14, 2018

jia200x added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 14, 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 14, 2018

jia200x requested changes Dec 14, 2018

View reviewed changes

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

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

miri64 and others added 5 commits December 14, 2018 19:05

gnrc_ipv6_ext: add missing newline to DEBUG

17eee21

gnrc_ipv6: move ext. hdr handling out of general demux

5c0d18b

They are handled separately anyway and this allows us to handle the Hop-by-hop option *before* forwarding in a later step.

gnrc_ipv6: handle hop-by-hop option before forwarding

e4fa143

gnrc_ipv6: dispatch ext. headers to external interested parties

0580fa5

This way e.g. a raw socket listening for an extension headers protocol number also get's it.

gnrc_ipv6_ext: make single header demux function private

bbcb316

The function is now only called by `gnrc_ipv6_ext_process_hopopt()` and `gnrc_ipv6_ext_process_all()`, both are part of the `gnrc_ipv6_ext` module.

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from 439d58d to d711cef Compare December 14, 2018 18:09

miri64 commented Dec 14, 2018

View reviewed changes

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

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

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from d711cef to c90f2c2 Compare December 14, 2018 18:12

RIOT-OS deleted a comment Dec 15, 2018

gnrc_ipv6_ext: rename nh to protnum for clarity

864ce4f

miri64 force-pushed the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch from c90f2c2 to 864ce4f Compare December 15, 2018 17:21

jia200x approved these changes Dec 17, 2018

View reviewed changes

jia200x merged commit 469c31c into RIOT-OS:master Dec 17, 2018

miri64 deleted the gnrc_ipv6/enh/take-out-hop-by-hop-ext branch December 17, 2018 09:25

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

	#define gnrc_icmpv6_error_dst_unr_send(code, orig_pkt) \
	(void)code; (void)orig_pkt
	#define gnrc_icmpv6_error_pkt_too_big_send(mtu, orig_pkt) \
	(void)mtu; (void)orig_pkt
	#define gnrc_icmpv6_error_time_exc_send(code, orig_pkt) \
	(void)code; (void)orig_pkt
	#define gnrc_icmpv6_error_param_prob_send(code, ptr, orig_pkt) \
	(void)code; (void)ptr, (void)orig_pkt

	#define gnrc_netif_ipv6_iid_from_addr(netif, addr, addr_len, iid) (-ENOTSUP)
	#define gnrc_netif_ipv6_iid_to_addr(netif, iid, addr) (-ENOTSUP)

Conversation

miri64 commented Oct 24, 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 29, 2018

Uh oh!

miri64 commented Oct 30, 2018

Uh oh!

miri64 commented Nov 8, 2018

Uh oh!

miri64 commented Nov 14, 2018

Uh oh!

miri64 commented Nov 17, 2018

Uh oh!

miri64 commented Dec 14, 2018

Uh oh!

miri64 commented Dec 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x commented Dec 14, 2018

Uh oh!

jia200x commented Dec 14, 2018

Uh oh!

jia200x commented Dec 14, 2018

Uh oh!

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miri64 commented Dec 14, 2018

Uh oh!

jia200x commented Dec 17, 2018

Uh oh!

miri64 commented Dec 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miri64 commented Oct 24, 2018 •

edited

Loading