gnrc ipv6: replace check by assert by OlegHahm · Pull Request #5179 · RIOT-OS/RIOT

OlegHahm · 2016-03-25T16:35:07Z

The existence of netif is mandatory here.

IMO this check doesn't make sense because https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L855 requires iface to be set. @authmillenon, can you verify?

The existence of netif is mandatory here.

miri64 · 2016-03-25T16:39:41Z

That might be, but I would like to keep it the way, that you can use IPv6 without a netif header.

OlegHahm · 2016-03-25T18:05:02Z

That might be, but I would like to keep it the way, that you can use IPv6 without a netif header.

You mean: "change it" instead of "keep it"?

OlegHahm · 2016-03-25T18:18:32Z

And how would one get the inferface without a netif header?

cgundogan · 2016-03-25T18:37:06Z

IMO this check doesn't make sense because https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L855 requires iface to be set. @authmillenon, can you verify?

I don't think that's quite true. _pkt_not_for_me can handle the case for KERNEL_PID_UNDEF.

miri64 · 2016-03-25T19:06:39Z

You mean: "change it" instead of "keep it"?

No, that's why the check is in there. IPv6 is (and should) be able to handle packets without netif header. Otherwise, we damage GNRC's modularity by making it de facto monolithic in a modular dress ;)

OlegHahm · 2016-03-25T20:40:25Z

I don't think that's quite true. _pkt_not_for_me can handle the case for KERNEL_PID_UNDEF.

True, haven't looked carefully. Thanks for the correction.

OlegHahm · 2016-03-25T20:42:48Z

IPv6 is (and should) be able to handle packets without netif header.

Can NDP work without the information about the interface of incoming interfaces?

Otherwise, we damage GNRC's modularity by making it de facto monolithic in a modular dress ;)

I fail to see the relationship. Upper layer require information from lower layers. UDP won't work without an IP header and IP needs information about the interfaces.

miri64 · 2016-03-26T08:50:44Z

Can NDP work without the information about the interface of incoming interfaces?

If there is only one interface, yes.

I fail to see the relationship. Upper layer require information from lower layers. UDP won't work without an IP header and IP needs information about the interfaces.

Not necessarily. Especially in the IPv6 case we only need this header to determine the interface PID which isn't even network related but purely OS related and there can also be other ways to determine the interface, if we really need it (e.g. searching the destination address on the interfaces). UDP is a different case because the (address, port) tuple defines its communication end points. And also: UDP doesn't necessarily need an IPv6 header. It also works with IPv4 and any other surrounding header. This interchangeability makes it as modular as IPv6, just in a different sense.

OlegHahm · 2016-03-26T09:50:47Z

I don't argue with that, I just say that the interface identifier should always be provided to the IP layer.

miri64 · 2016-03-26T10:28:35Z

Regardless, are we okay to close this PR?

OlegHahm · 2016-03-26T10:50:06Z

Do we have another solution?

miri64 · 2016-03-26T11:05:27Z

Solution for what? I still don't see any problem here.

OlegHahm · 2016-03-26T11:35:13Z

Can NDP work without the information about the interface of incoming interfaces?

If there is only one interface, yes.

I took this statement as an implicit "won't work for multiple interfaces without netif header". And more general, I think any protocol that works on the network layer will require information about the interfaces. At least IP requires this for the neighbor/ARP cache and CCN/NDN requires it for the reverse path forwarding.

IMO we can either go with a mandatory netif header (without changing much in the code) or provide some other mechanism to obtain this information.

miri64 · 2016-03-26T11:41:11Z

How about this: For one interface the netif header is not mandatory. For more then one it is. This way we don't need to change any code.

miri64 · 2016-03-26T11:42:15Z

(I'm still doubting that it even is mandatory for multiple interfaces, because _pkt_not_for_me is returning the iface PID, too).

OlegHahm · 2016-03-26T13:54:50Z

How about this: For one interface the netif header is not mandatory. For more then one it is. This way we don't need to change any code.

Wouldn't this require an (ugly) ifdef?

I'm still doubting that it even is mandatory for multiple interfaces, because _pkt_not_for_me is returning the iface PID, too.

The code there seems to be more like wild guessing.

miri64 · 2016-03-26T15:09:32Z

Okay, let's go back a step... Is there a problem? If yes: Does this PR fixes it? And how?

OlegHahm · 2016-03-26T17:03:16Z

The only concrete problem that I have observed and which would be fixed by this PR is in #5178. One cannot update the IPv6 RX statistics per interface if one doesn't which interface received the datagram. For NDP in the multi-interface case I haven't traced what would happen if the interface information is missing, but I assume that it might lead to wrong results. If I see it correctly, _packet_not_for_me() "guesses" the interface based on the destination IP address. Is that correct?

OlegHahm · 2016-03-26T17:05:00Z

On the other hand, is there any case where we don't pass a netif header to IPv6?

miri64 · 2016-03-26T17:23:36Z

On the other hand, is there any case where we don't pass a netif header to IPv6?

Currently not, but I would like to keep enforced crashes (which asserts basically are :P) to a minimum :P.

If I see it correctly, _packet_not_for_me() "guesses" the interface based on the destination IP address. Is that correct?

That is correct.

miri64 · 2016-03-26T17:24:05Z

(but at least for unicast addresses this should be a very good guess).

OlegHahm · 2016-03-26T17:37:43Z

On the other hand, is there any case where we don't pass a netif header to IPv6?

Currently not, but I would like to keep enforced crashes (which asserts basically are :P) to a minimum :P.

Yes, enforced and controlled crashes during testing and development. That's basically the purpose of asserts. And I definitely prefer this over potentially uncontrolled behavior.

Can you envision any case where passing a netif header from any lower layer would conflict with anything?

OlegHahm · 2016-03-26T17:40:52Z

(but at least for unicast addresses this should be a very good guess).

Not so sure. Currently any IEEE 802.15.4 transceiver without hardware addresses stored on the transceiver itself (i.e. at86rf2xx or kw2xrf) would get the same link-local address. (Which itself is not perfect, but AFAIK legal behavior, since whether link-layer nor link-local IP addresses are required to be unique beyond the scope of the link.)

miri64 · 2016-03-26T17:59:20Z

Can you envision any case where passing a netif header from any lower layer would conflict with anything?

Especially for testing I could envision such a case, or stuff like SLIP where addresses are not required.

miri64 · 2016-03-26T18:00:29Z

Not so sure. Currently any IEEE 802.15.4 transceiver without hardware addresses stored on the transceiver itself (i.e. at86rf2xx or kw2xrf) would get the same link-local address. (Which itself is not perfect, but AFAIK legal behavior, since whether link-layer nor link-local IP addresses are required to be unique beyond the scope of the link.)

This is something DAD should fix very soon (I will take DAD into account for my ND remodelling): If an address already exists: change hardware address.

miri64 · 2016-03-26T18:17:35Z

(Which itself is not perfect, but AFAIK legal behavior, since whether link-layer nor link-local IP addresses are required to be unique beyond the scope of the link.)

And GUAs should have different prefixes for different interfaces ;-)

OlegHahm · 2016-03-26T18:56:48Z

This is something DAD should fix very soon (I will take DAD into account for my ND remodelling): If an address already exists: change hardware address.

Only if the operate in the same network. (Which wouldn't make too much sense for two IEEE 802.15.4 transceivers.)

OlegHahm · 2016-03-26T19:00:07Z

Especially for testing I could envision such a case, or stuff like SLIP where addresses are not required.

For testing a netif header can be easily mocked (and I'm unwilling to make any compromises just for testing) and SLIP may not require L2 addresses, but still run over a certain interface. As I wrote above: we could also introduce a separate mechanism to obtain the receiving interface, but I think just filling and passing the netif header would be easier.

miri64 · 2016-03-29T15:05:03Z

Will test at the Hack'n'ACK. Please remind me!

cgundogan · 2016-03-29T15:50:14Z

I didn't check the docs, but I assume they will need to be updated to include this enforcement of netif headers.

miri64 · 2016-03-29T15:58:11Z

Yapp.

OlegHahm · 2016-03-29T16:44:05Z

Would this line of documentation be okay?

cgundogan · 2016-03-29T16:47:13Z

untested ACK

@authmillenon

Will test at the Hack'n'ACK. Please remind me!

Did you?

miri64 · 2016-03-29T17:32:49Z

gnrc_networking is still working. ACK.

kaspar030 · 2016-04-04T20:24:25Z

This breaks PR #4725, no idea why. Somehow the assertion fails.

kaspar030 · 2016-04-04T20:26:40Z

----> ethos: hello received
uhcpc: Using 7 as border interface and 6 as wireless interface.
uhcp_client(): sending REQ...
main(): This is RIOT! (Version: 2016.03-devel-1605-gbbd90-booze-ethos_br_hack)
RIOT border router example applice1f:19got packet from fe80::dcf2:5cff:fe1f:19b8 port 60118
uhcp: push from fe80::dcf2:5cff:fe1f:19b8:60118 prefix=2001:db8::/64
0x2def
*** RIOT kernel panic:
FAILED ASSERTION.

        pid | name                 | state    Q | pri | stack ( used) | location
          1 | idle                 | pending  Q |  15 |   256 (  128) | 0x20000e34
          2 | main                 | pending  Q |   7 |  1536 (  552) | 0x20000f34
          3 | 6lo                  | bl rx    _ |   3 |  1024 (  460) | 0x200047d0
          4 | ipv6                 | running  Q |   4 |  1024 (  668) | 0x200032c4
          5 | udp                  | bl rx    _ |   5 |  1024 (  348) | 0x20004fe8
          6 | at86rf2xx            | bl rx    _ |   3 |  1024 (  380) | 0x20002e44
          7 | gnrc_ethos           | bl rx    _ |   3 |  1024 (  352) | 0x20000224
          8 | uhcp                 | pending  Q |   6 |  1536 (  936) | 0x200053ec
            | SUM                  |            |     |  8448 ( 3824)

*** halted.

Debugger shows assertion fails here: https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L768

miri64 · 2016-04-04T23:01:16Z

Maybe your PR is broken ;-P do you prepend the netif header in ethos as expected per this PR?

(Hi, I'm a Troubleshooting person. Have you tried turning it on and off again ;-P)

OlegHahm · 2016-04-04T23:09:39Z

No, the missing netif header occurs actually in a loopback case. (Why there's loopback communication at all is another mistery.)

kaspar030 · 2016-04-04T23:11:12Z

Maybe your PR is broken ;-P do you prepend the netif header in ethos as expected per this PR?

ethos doesn't deal with netif... Somehow there are two packets without netif header that only appear if uhcpc adds the prefix. I think the packets get triggered by router advertisement code.

OlegHahm · 2016-04-04T23:11:33Z

Currently I try to understand why we cannot simply reverse the pktsnip order here.

gnrc_ipv6: Revert #5179

gnrc_ipv6: Revert #5179 (backport)

gnrc ipv6: replace check by assert

856e1ee

The existence of netif is mandatory here.

OlegHahm assigned miri64 Mar 25, 2016

OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: question The issue poses a question regarding usage of RIOT Area: network Area: Networking GNRC labels Mar 25, 2016

OlegHahm added this to the Release 2016.04 milestone Mar 25, 2016

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Mar 29, 2016

doc: mandatory netif snip for gnrc_ipv6 receive()

f15e038

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 29, 2016

miri64 merged commit 6504671 into RIOT-OS:master Mar 29, 2016

OlegHahm deleted the ipv6_recv_assert_instead_check branch March 29, 2016 18:33

kaspar030 mentioned this pull request Apr 4, 2016

simplified border router setup #4725

Merged

miri64 mentioned this pull request Apr 5, 2016

gnrc ipv6: append empty netif hdr for loopback #5247

Closed

OlegHahm mentioned this pull request Apr 9, 2016

gnrc ipv6: sending over loopback is broken #5279

Closed

miri64 mentioned this pull request Apr 17, 2016

gnrc_ipv6: Revert #5179 #5326

Merged

OlegHahm mentioned this pull request Apr 20, 2016

net: introduce IPv6 netstats #5178

Merged

haukepetersen added a commit that referenced this pull request Apr 20, 2016

Merge pull request #5326 from authmillenon/gnrc_ipv6/fix/revert5179

4ace701

gnrc_ipv6: Revert #5179

cgundogan added a commit that referenced this pull request Apr 20, 2016

Merge pull request #5370 from authmillenon/gnrc_ipv6/fix/revert5179-bp

e302d2a

gnrc_ipv6: Revert #5179 (backport)

Uh oh!

Conversation

OlegHahm commented Mar 25, 2016

Uh oh!

miri64 commented Mar 25, 2016

Uh oh!

OlegHahm commented Mar 25, 2016

Uh oh!

OlegHahm commented Mar 25, 2016

Uh oh!

cgundogan commented Mar 25, 2016

Uh oh!

miri64 commented Mar 25, 2016

Uh oh!

OlegHahm commented Mar 25, 2016

Uh oh!

OlegHahm commented Mar 25, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

miri64 commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

OlegHahm commented Mar 26, 2016

Uh oh!

miri64 commented Mar 29, 2016

Uh oh!

cgundogan commented Mar 29, 2016

Uh oh!

miri64 commented Mar 29, 2016

Uh oh!

OlegHahm commented Mar 29, 2016

Uh oh!

cgundogan commented Mar 29, 2016

Uh oh!

miri64 commented Mar 29, 2016

Uh oh!

kaspar030 commented Apr 4, 2016

Uh oh!

kaspar030 commented Apr 4, 2016

Uh oh!

miri64 commented Apr 4, 2016

Uh oh!

OlegHahm commented Apr 4, 2016

Uh oh!