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.

Sign up for GitHub

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

gnrc_ipv6_nib: clean up _resolve_addr() #18939

Merged
merged 3 commits into from
Jan 10, 2023
Merged

gnrc_ipv6_nib: clean up _resolve_addr() #18939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0886ad4
gnrc_ipv6_nib: factor out resolve from nc
benpicco Nov 20, 2022
2e2414c
gnrc_ipv6_nib: factor out enqueue on resolve
benpicco Nov 20, 2022
64a2fbc
gnrc_ipv6_nib: clean up _resolve_addr()
benpicco Nov 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ bool _is_reachable(_nib_onl_entry_t *entry);
#define _set_reachable(netif, nce) (void)netif; (void)nce

#define _get_nud_state(nbr) (GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNMANAGED)
#define _set_nud_state(netif, nce, state) (void)netif; (void)nbr; (void)state
#define _set_nud_state(netif, nce, state) (void)netif; (void)nce; (void)state
#define _is_reachable(entry) (true)
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM || defined(DOXYGEN) */

Expand Down
199 changes: 106 additions & 93 deletions sys/net/gnrc/network_layer/ipv6/nib/nib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1251,25 +1251,88 @@ static void _handle_nbr_adv(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
}
}

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
static gnrc_pktqueue_t *_alloc_queue_entry(gnrc_pktsnip_t *pkt)
{
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
for (int i = 0; i < CONFIG_GNRC_IPV6_NIB_NUMOF; i++) {
if (_queue_pool[i].pkt == NULL) {
_queue_pool[i].pkt = pkt;
return &_queue_pool[i];
}
}
#else
(void)pkt;
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
return NULL;
}
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */

static bool _resolve_addr_from_nc(_nib_onl_entry_t *entry, gnrc_netif_t *netif,
gnrc_ipv6_nib_nc_t *nce)
{
if (entry == NULL) {
return false;
}

if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {
if (!_is_reachable(entry)) {
return false;
}

if (_get_nud_state(entry) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_STALE) {
_set_nud_state(netif, entry, GNRC_IPV6_NIB_NC_INFO_NUD_STATE_DELAY);
_evtimer_add(entry, GNRC_IPV6_NIB_DELAY_TIMEOUT,
&entry->nud_timeout, NDP_DELAY_FIRST_PROBE_MS);
}
}

_nib_nc_get(entry, nce);
return true;
}

static bool _enqueue_for_resolve(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt,
_nib_onl_entry_t *entry)
{
if (!IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT) ||
_get_nud_state(entry) != GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE) {
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR, pkt);
gnrc_pktbuf_release_error(pkt, EHOSTUNREACH);
return true;
}

gnrc_pktqueue_t *queue_entry = _alloc_queue_entry(pkt);

if (queue_entry == NULL) {
DEBUG("nib: can't allocate entry for packet queue "
"dropping packet\n");
gnrc_pktbuf_release(pkt);
return false;
}

if (netif != NULL) {
gnrc_pktsnip_t *netif_hdr = gnrc_netif_hdr_build(NULL, 0, NULL, 0);

if (netif_hdr == NULL) {
DEBUG("nib: can't allocate netif header for queue\n");
gnrc_pktbuf_release(pkt);
queue_entry->pkt = NULL;
return false;
}
gnrc_netif_hdr_set_netif(netif_hdr->data, netif);
queue_entry->pkt = gnrc_pkt_prepend(queue_entry->pkt, netif_hdr);
}

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
gnrc_pktqueue_add(&entry->pktqueue, queue_entry);
#else
(void)entry;
#endif
return true;
}

static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
gnrc_pktsnip_t *pkt, gnrc_ipv6_nib_nc_t *nce,
_nib_onl_entry_t *entry)
{
bool res = false;

if ((netif != NULL) && (netif->device_type == NETDEV_TYPE_SLIP)) {
/* XXX: Linux doesn't do neighbor discovery for SLIP so no use sending
* NS and since SLIP doesn't have link-layer addresses anyway, we can
Expand All @@ -1279,107 +1342,57 @@ static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
nce->l2addr_len = 0;
return true;
}
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)
if ((entry != NULL) && _is_reachable(entry)) {
if (_get_nud_state(entry) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_STALE) {
_set_nud_state(netif, entry, GNRC_IPV6_NIB_NC_INFO_NUD_STATE_DELAY);
_evtimer_add(entry, GNRC_IPV6_NIB_DELAY_TIMEOUT,
&entry->nud_timeout, NDP_DELAY_FIRST_PROBE_MS);
}

/* first check if address is cached */
if (_resolve_addr_from_nc(entry, netif, nce)) {
DEBUG("nib: resolve address %s%%%u from neighbor cache\n",
ipv6_addr_to_str(addr_str, &entry->ipv6, sizeof(addr_str)),
_nib_onl_get_if(entry));
_nib_nc_get(entry, nce);
res = true;
return true;
}
#else /* CONFIG_GNRC_IPV6_NIB_ARSM */
if (entry != NULL) {
DEBUG("nib: resolve address %s%%%u from neighbor cache\n",
ipv6_addr_to_str(addr_str, &entry->ipv6, sizeof(addr_str)),
_nib_onl_get_if(entry));
_nib_nc_get(entry, nce);
res = true;

/* directly resolve address if it uses 6lo addressing mode */
if (_resolve_addr_from_ipv6(dst, netif, nce)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, this if clause could use a nice DEBUG() message too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this should be moved above _resolve_addr_from_nc()

Copy link
Contributor

Choose a reason for hiding this comment

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

could be, maybe, but not necessarily.

static bool _resolve_addr_from_nc(gnrc_netif_t *netif, gnrc_ipv6_nib_nc_t *nce, const ipv6_addr_t * dst) {
    /* lookup _nib_onl_entry_t *entry by dst and netif */
    /* if exists get L2 address from NC */
    /* else if 6LN and link-local do reverse translation */
}

And if _resolve_addr_from_nc() fails to resolve the L2 address, we know we don´t have an NC entry and we have to add one, or it existed but was UNREACHABLE. Also having (netif + dst) and entry as input is a bit redundant.
... I don´t know, maybe the clean up could propagate upwards, but not without changing too much I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you want me to do 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I misread your last comment. I thought you wanted to merge _resolve_addr_from_nc() and _resolve_addr_from_ipv6() into one function. Then I thought about how this would look like.

Ok, moving _resolve_addr_from_ipv6() above _resolve_addr_from_nc() and keeping them as they are now,
would mean that every link-local destination address would be resolved immediately by reverse translation.
This would mean that link local addresses would not have to be registered. But this is somewhat required if the link local address is a private address (CGA or SLAAC pivacy extension) .

Let´s say R is the upstream router of host H. H implements the SLAAC privacy extension and builds a private link local address that it registers to R where due to an SLLA option an NCA is created there.
If R would not examine the NCE when it sends something to the private SLAAC address of H, it would put the wrong L2 address in the frame.

In fact, in my SEND branch, I added this:

    /* If any IPv6 privacy extension is used, we cannot derive the L2 address from the IP address */
    bool res = !IS_USED(MODULE_IPV6_CGA) &&
               (netif != NULL) &&
               gnrc_netif_is_6ln(netif) &&
               ipv6_addr_is_link_local(dst);

to _resolve_addr_from_ipv6().

DEBUG("nib: resolve l2 address from IPv6 address\n");
return true;
}
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM */
else if (!(res = _resolve_addr_from_ipv6(dst, netif, nce))) {
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)
bool reset = false;
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM */

DEBUG("nib: resolve address %s by probing neighbors\n",
ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
bool reset = false;
DEBUG("nib: resolve address %s by probing neighbors\n",
ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
if (entry == NULL) {
entry = _nib_nc_add(dst, netif ? netif->pid : 0,
GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
if (entry == NULL) {
entry = _nib_nc_add(dst, (netif != NULL) ? netif->pid : 0,
GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
if (entry == NULL) {
DEBUG("nib: can't resolve address, neighbor cache full\n");
gnrc_pktbuf_release(pkt);
return false;
}
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ROUTER)
if (netif != NULL) {
_call_route_info_cb(netif,
GNRC_IPV6_NIB_ROUTE_INFO_TYPE_NSC,
dst,
(void *)GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
}
#endif /* CONFIG_GNRC_IPV6_NIB_ROUTER */
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)
reset = true;
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM */
DEBUG("nib: can't resolve address, neighbor cache full\n");
gnrc_pktbuf_release(pkt);
return false;
}
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)
else if (_get_nud_state(entry) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE) {
/* reduce back-off to possibly resolve neighbor sooner again */
entry->ns_sent = 3;
}
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM */
if (pkt != NULL) {
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_QUEUE_PKT)
if (_get_nud_state(entry) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE) {
gnrc_pktqueue_t *queue_entry = _alloc_queue_entry(pkt);

if (queue_entry != NULL) {
if (netif != NULL) {
gnrc_pktsnip_t *netif_hdr = gnrc_netif_hdr_build(
NULL, 0, NULL, 0
);
if (netif_hdr == NULL) {
DEBUG("nib: can't allocate netif header for queue\n");
gnrc_pktbuf_release(pkt);
queue_entry->pkt = NULL;
return false;
}
gnrc_netif_hdr_set_netif(netif_hdr->data, netif);
queue_entry->pkt = gnrc_pkt_prepend(queue_entry->pkt,
netif_hdr);
}
gnrc_pktqueue_add(&entry->pktqueue, queue_entry);
}
else {
DEBUG("nib: can't allocate entry for packet queue "
"dropping packet\n");
gnrc_pktbuf_release(pkt);
return false;
}
}
/* pkt != NULL already checked above */
else {
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR,
pkt);
gnrc_pktbuf_release_error(pkt, EHOSTUNREACH);
}
#else /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR,
pkt);
gnrc_pktbuf_release_error(pkt, EHOSTUNREACH);
#endif /* CONFIG_GNRC_IPV6_NIB_QUEUE_PKT */
if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ROUTER) && netif != NULL) {
_call_route_info_cb(netif,
GNRC_IPV6_NIB_ROUTE_INFO_TYPE_NSC,
dst,
(void *)GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
}
reset = true;
}
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)
else if (_get_nud_state(entry) == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE) {
/* reduce back-off to possibly resolve neighbor sooner again */
entry->ns_sent = 3;
}
#endif

/* queue packet as we have to do address resolution first */
if (pkt != NULL && !_enqueue_for_resolve(netif, pkt, entry)) {
return false;
}

if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {
Copy link
Contributor Author

@benpicco benpicco Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have a feeling that this should in fact be

Suggested change
if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM)) {
if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ARSM) && !gnrc_netif_is_6lo(netif)) {

but I'll leave this PR to cleanups only.

Copy link
Member

Choose a reason for hiding this comment

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

No, it shoud still be possible to activate the ARSM optionally on 6Lo nodes.

Copy link
Member

Choose a reason for hiding this comment

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

(also resolution by address generation reversal is also a nice feature for non-6Lo-nodes to have (and will become necessary with SCHC at some point too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shoud still be possible to activate the ARSM optionally on 6Lo nodes.

But that would be a separate config then, right? Because right now we only have CONFIG_GNRC_IPV6_NIB_ARSM which globally turns on ARSM on all interfaces.

So the BR will do ARSM on the 6lo interface (because it needs it on the upstream interface) but the 6lo nodes do not.

Copy link
Contributor

@fabian18 fabian18 Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, CONFIG_GNRC_IPV6_NIB_ARSM should even be enabled for all 6LN. Because our CONFIG_GNRC_IPV6_NIB_ARSM is actually responsible for two things:

  • address resolution obviously
    (Note: with private addresses, 6LN cannot reverse translate IPv6 addresses to L2 addresses, so they must look in the NC anyway. But this is a bit off-topic here currently.)
  • and Neighbor Unreachable Detection

6LN actually do NUD and additionally include an ARO option RFC6775.

Hosts send unicast NS messages to register their IPv6 addresses, and
also to do NUD to verify that their default routers are still
reachable.

For some things like joining the Solicited-node multicast address and multicast L2 address resolution, we just exclude 6LN interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neighbor Unreachable Detection

hm might this be the reason for #17327

Copy link
Contributor

Choose a reason for hiding this comment

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

very hard to tell... To be honest I am not very familiar with RPL. I have to catch up here.

_probe_nbr(entry, reset);
#endif /* CONFIG_GNRC_IPV6_NIB_ARSM */
}
return res;

return false;
}

static void _handle_snd_na(gnrc_pktsnip_t *pkt)
Expand Down