[riot-notifications] [RIOT-OS/RIOT] gnrc_ipv6_nib: add full RFC4862 DAD support (#8823)

benpicco notifications at github.com
Tue Jun 15 16:06:31 CEST 2021


@benpicco commented on this pull request.



> @@ -838,9 +837,30 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
     DEBUG("     - Destination address: %s\n",
           ipv6_addr_to_str(addr_str, &ipv6->dst, sizeof(addr_str)));
 #if GNRC_IPV6_NIB_CONF_SLAAC
-    /* TODO SLAAC behavior */
+    gnrc_netif_t *tgt_netif = gnrc_netif_get_by_ipv6_addr(&nbr_sol->tgt);

No it is coming over interface `7`, but the source tries to reach the address on interface `6`.

I now have a fix that works for this case, but it reveals a flaw somewhere else:
Now the NS is answered for all interfaces, but why is the neighbor sending a NS for the second interface in the first place when it should instead just route the packet to the next hop (`2001:16b8:45b5:9af8:5884:3bff:fe4f:a903%7`).

<details><summary>the fix</summary>
```patch
diff --git a/sys/net/gnrc/network_layer/ipv6/nib/nib.c b/sys/net/gnrc/network_layer/ipv6/nib/nib.c
index 856a536735..93faccfaae 100644
--- a/sys/net/gnrc/network_layer/ipv6/nib/nib.c
+++ b/sys/net/gnrc/network_layer/ipv6/nib/nib.c
@@ -41,7 +41,7 @@
 #include "_nib-6lr.h"
 #include "_nib-slaac.h"
 
-#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1
 #include "debug.h"
 
 #if IS_ACTIVE(ENABLE_DEBUG)
@@ -940,6 +940,7 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
                             const ndp_nbr_sol_t *nbr_sol, size_t icmpv6_len)
 {
     size_t tmp_len = icmpv6_len - sizeof(ndp_nbr_sol_t);
+    gnrc_netif_t *tgt_netif;
     int tgt_idx;
     ndp_opt_t *opt;
 
@@ -965,8 +966,16 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
               ipv6_addr_to_str(addr_str, &ipv6->dst, sizeof(addr_str)));
         return;
     }
-    /* check if target is assigned only now in case the length was wrong */
-    tgt_idx = gnrc_netif_ipv6_addr_idx(netif, &nbr_sol->tgt);
+
+    tgt_netif = gnrc_netif_get_by_ipv6_addr(&nbr_sol->tgt);
+
+    if (tgt_netif != NULL) {
+        /* check if target is assigned only now in case the length was wrong */
+        tgt_idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_sol->tgt);
+    } else {
+        tgt_idx = -1;
+    }
+
     if (tgt_idx < 0) {
         DEBUG("nib: Target address %s is not assigned to the local interface\n",
               ipv6_addr_to_str(addr_str, &nbr_sol->tgt, sizeof(addr_str)));
@@ -993,19 +1002,16 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
     DEBUG("     - Destination address: %s\n",
           ipv6_addr_to_str(addr_str, &ipv6->dst, sizeof(addr_str)));
 #if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_SLAAC)
-    gnrc_netif_t *tgt_netif = gnrc_netif_get_by_ipv6_addr(&nbr_sol->tgt);
 
     if (tgt_netif != NULL) {
-        int idx;
-
         gnrc_netif_acquire(tgt_netif);
-        idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_sol->tgt);
+        tgt_idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_sol->tgt);
         /* if idx < 0:
          * nbr_sol->tgt was removed between getting tgt_netif by nbr_sol->tgt
          * and gnrc_netif_acquire(tgt_netif). This is like `tgt_netif` would
          * have been NULL in the first place so just continue as if it would
          * have. */
-        if ((idx >= 0) && gnrc_netif_ipv6_addr_dad_trans(tgt_netif, idx)) {
+        if ((tgt_idx >= 0) && gnrc_netif_ipv6_addr_dad_trans(tgt_netif, tgt_idx)) {
             if (!ipv6_addr_is_unspecified(&ipv6->src)) {
                 /* (see https://tools.ietf.org/html/rfc4862#section-5.4.3) */
                 DEBUG("nib: Neighbor is performing AR, but target address is "
@@ -1015,7 +1021,7 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
             }
             /* cancel validation timer */
             evtimer_del(&_nib_evtimer,
-                        &tgt_netif->ipv6.addrs_timers[idx].event);
+                        &tgt_netif->ipv6.addrs_timers[tgt_idx].event);
             /* _remove_tentative_addr() context switches to `tgt_netif->pid` so
              * release `tgt_netif`. We are done here anyway. */
             gnrc_netif_release(tgt_netif);
@@ -1026,7 +1032,7 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
     }
 #endif  /* CONFIG_GNRC_IPV6_NIB_SLAAC */
     if (ipv6_addr_is_unspecified(&ipv6->src)) {
-        gnrc_ndp_nbr_adv_send(&nbr_sol->tgt, netif, &ipv6->src, false, NULL);
+        gnrc_ndp_nbr_adv_send(&nbr_sol->tgt, tgt_netif, &ipv6->src, false, NULL);
     }
     else {
         gnrc_pktsnip_t *reply_aro = NULL;
@@ -1072,10 +1078,10 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
         reply_aro = _copy_and_handle_aro(netif, ipv6, nbr_sol, aro, sl2ao);
         /* check if target address is anycast */
         if (netif->ipv6.addrs_flags[tgt_idx] & GNRC_NETIF_IPV6_ADDRS_FLAGS_ANYCAST) {
-            _send_delayed_nbr_adv(netif, &nbr_sol->tgt, ipv6, reply_aro);
+            _send_delayed_nbr_adv(tgt_netif, &nbr_sol->tgt, ipv6, reply_aro);
         }
         else {
-            gnrc_ndp_nbr_adv_send(&nbr_sol->tgt, netif, &ipv6->src,
+            gnrc_ndp_nbr_adv_send(&nbr_sol->tgt, tgt_netif, &ipv6->src,
                                   ipv6_addr_is_multicast(&ipv6->dst),
                                   reply_aro);
         }
```
</details>

This falls apart when I try to reach nodes connected to `6` since the source still sends a NS to the intermediate node which it now can of course not answer. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/8823#discussion_r651826570
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210615/87ecd82a/attachment.htm>


More information about the notifications mailing list