Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ng_ndp: initial import of address resolution #2910

Merged
merged 2 commits into from
May 17, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 3, 2015

This imports address resolution and neighbor unreachable discovery for IPv6.

Depends on #2908 (merged), #2909 (merged) and #2964 (merged).

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first NSTF labels May 3, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone May 3, 2015
@miri64 miri64 force-pushed the ng_ndp/feat/addr-res branch 2 times, most recently from 78a52fb to 4f54b07 Compare May 10, 2015 15:46
@miri64
Copy link
Member Author

miri64 commented May 10, 2015

Rebased to current master and current dependencies

@miri64
Copy link
Member Author

miri64 commented May 10, 2015

Added #2964 as dependency

@miri64
Copy link
Member Author

miri64 commented May 11, 2015

New fixes.

@miri64 miri64 force-pushed the ng_ndp/feat/addr-res branch 3 times, most recently from 0b6af41 to 56c3d09 Compare May 12, 2015 17:34
@miri64
Copy link
Member Author

miri64 commented May 12, 2015

Rebased to current dependencies

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 12, 2015
@miri64
Copy link
Member Author

miri64 commented May 12, 2015

(no longer WIP, but still untested)

@miri64
Copy link
Member Author

miri64 commented May 13, 2015

Tested and fixed bugs I found. Please test (hope there are devices to test this with...).

@miri64
Copy link
Member Author

miri64 commented May 13, 2015

(hope there are devices to test this with...).

Problems I ran into so far with current devices:

The way I did it:

Great now I know at least, that the initial probing works, but how do I test the receiving side:

  • the answer is scapy: when I hit [Enter] on ping6 1 fe80::1 I rapidly change to an already started scapy session (needs to be started with root previleges) and hit the following command (which I prepared too of course, since it needs to be in reply of one of the Neighbor solicitations) to send a corresponding Neighbor Advertisement:
sendp(Ether(src="<the tap's MAC address>", dst="<RIOT's MAC>") / \
      IPv6(src="fe80::1", dst="RIOT's IPv6 address") / \
      ICMPv6ND_NA(tgt="fe80::1",O=0,R=0,S=1) / \
      ICMPv6NDOptDstLLAddr(lladdr="<the tap's MAC address>"), \
      iface="<the name of the tap>")

Both RIOT's MAC and IPv6 address can be determined with ifconfig.

  • There will be only Neighbor Solicitations as long as no Neighbor Advertisement was received, so don't be shocked if you see less than 3 NS ;-)
  • If the NA was received correctly you should see with ncache that the address fe80::1 is indeed REACHABLE.
  • If you wait 2.5 to 7.5 sec (the device chooses a random value in this range) the address should go STALE.
  • Sending another packet to fe80::1 will now trigger the address to enter DELAY state and to trigger PROBE state NG_NDP_FIRST_PROBE_DELAY seconds (5 seconds) afterwards. Check it with another ping6 1 fe80::1 and ncache (you might not be able to even see the DELAY state, since the ping6 application is waiting 5 seconds for a reply).
  • In PROBE state the node starts to send out Neighbor Solicitations again. This time they are addressed to fe80::1 and there will be NG_NDP_MAX_UC_NBR_SOL_NUMOF of them (which is normally 3 too).
  • If the Neighbor Solicitations stay unanswered the entry will be removed from the Neighbor cache (the node is to be determined unreachable). Check with ncache.

@miri64
Copy link
Member Author

miri64 commented May 13, 2015

If you want to test the receiving of Neighbor Solicitations you can do that with scapy, too:

srp(Ether(src="<the tap's MAC address>", dst="<RIOT's MAC>") / \
    IPv6(src="fe80::1", dst="ff02::1") / \
    ICMPv6ND_NS(tgt="fe80::1") / \
    ICMPv6NDOptSrcLLAddr(lladdr="<the tap's MAC address>"), \
    iface="<the name of the tap>")

Instead of ff02::1 you can also go more correctly and use the solicited-nodes address corresponding to RIOT's IPv6 address (for simplicity sake I just let it at all-nodes). Any legal multicast address the interface is joined to is accepted, the solicited-nodes address is only the preferred one.

You will observe a Neighbor Advertisement being replied. The neighbor cache entry is first set to STALE by the receiving of the Neighbor Solicitation then, due to sending the unicast Neighbor Advertisement to fe80::1 directly put to DELAY and later reaches the PROBE phase again, leading again to 3 unicast NS.

scapy however will show the following, since it got the expected NA:

Begin emission:
Finished to send 1 packets.
*
Received 1 packets, got 1 answers, remaining 0 packets
(<Results: TCP:0 UDP:0 ICMP:0 Other:1>, <Unanswered: TCP:0 UDP:0 ICMP:0 Other:0>)

@OlegHahm
Copy link
Member

ng_nativenet: works like a charm, but Linux failed to answer (I think this is a configuration problem since I still can't get two ng_nativenet nodes get to communicate either)

What do you mean? Works like charm, but does neither work with Linux nor between two RIOT native instances?

@miri64
Copy link
Member Author

miri64 commented May 13, 2015

What do you mean? Works like charm, but does neither work with Linux nor between two RIOT native instances?

See my [edit: test] descriptions below ;-) As I said: I think it's more a configuring problem of the TAP, than the implementation, since the packets seem to be correct.

@OlegHahm
Copy link
Member

It's seems to be a race: contiki-os/contiki#1063 (comment) 😉

@miri64
Copy link
Member Author

miri64 commented May 13, 2015

Unfair! They have a headstart :D

router = ng_ipv6_nc_get_next_router(router);

if (router == NULL) { /* still nothing found => no router in list */
return NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ugly, but I don't have a better idea for now. Maybe introduce an is_empty() function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for this one use-case? Seems inefficient to me... Especially since the code would basically look the same, just with an extra && !ng_ipv6_nc_is_empty() in l320, if I see this correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes... But you agree that this double check looks weird, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but is_empty() is not the semantic we look for here. The router list is more like a circular list which is costly compared to the current array-list implementation, so …

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but if both calls return NULL the list is empty, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes… but this is not how this is to be used here: the first ng_ipv6_nc_get_next_router() tries to round-robin the routers. Problem: since the router list (a subset of the neighbor cache) is an array list, the function returns NULL for both cases that the list is empty or the list has reached its end. The second call than checks if the list was empty or not and additionally retrieves a router if it was not empty. This means, unless I am mistaken: checking if the list was empty additionally would generate extra overhead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, it would probably introduce more overhead.

@miri64 miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 16, 2015
ng_pktsnip_t *pkt = ng_ndp_opt_build(type, sizeof(ng_ndp_opt_t) + l2addr_len,
next);

if (pkt == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (pkt != NULL) ... here, too.

@OlegHahm
Copy link
Member

If comments are adressed: ACK

@miri64
Copy link
Member Author

miri64 commented May 16, 2015

Rebased to current master and squashed

@miri64
Copy link
Member Author

miri64 commented May 16, 2015

(and addressed rest of the comments)

@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 16, 2015
@miri64
Copy link
Member Author

miri64 commented May 17, 2015

Just a little fix so the unittests would still compile

@kaspar030
Copy link
Contributor

For routing we need a working routing protocol and router discovery first ;-).

What happened to static routing? If I add 2001:db8::1/64 to an interface, there should be some static routing entry, and RIOT should use ndp to find the link local address. No need for a routing protocol or router discovery here?

@miri64
Copy link
Member Author

miri64 commented May 17, 2015

@kaspar030 if you compile with the FIB and you add an entry to the FIB it will be able to route to this entry, so yes: static routing is possible.

miri64 added a commit that referenced this pull request May 17, 2015
ng_ndp: initial import of address resolution
@miri64 miri64 merged commit 4f1d7ad into RIOT-OS:master May 17, 2015
@miri64 miri64 deleted the ng_ndp/feat/addr-res branch May 17, 2015 08:45
ng_ipv6_addr_t dst;

DEBUG("ndp: Retransmit neighbor solicitation for %s\n",
ng_ipv6_addr_to_str(addr_str, nc_entry->ipv6_addr, sizeof(addr_str)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addr_str is not defined, i.e. array is missing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2nd argument needs to be a pointer. Here and some lines below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants