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

Address registration handling inappropriate #15867

Open
tinstructor opened this issue Jan 27, 2021 · 7 comments
Open

Address registration handling inappropriate #15867

tinstructor opened this issue Jan 27, 2021 · 7 comments
Assignees
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@tinstructor
Copy link

tinstructor commented Jan 27, 2021

Description

There are a few situations wherein the default behavior of a 6LR for handling an incoming registration attempt (i.e., processing a valid NS + ARO and SLLAO) seems inappropriate. For example, when a 6LR receives a valid NS with an ARO and SLLAO, and the ARO's Registration Lifetime is zero but the 6LR finds no matching NCE in its neighbor cache, it would make sense to simply ignore the NS (this is however debatable). Instead, it seems that a NA is returned, but without an ARO. This behavior does not make sense in any case. If anything, when returning a NA in this case, it should at least contain an ARO with Status code 0, or a NA should not be sent in response at all. A similar problem occurs when a new NCE is to be created but the cache is full.

Steps to reproduce the issue

In a first step _handle_nbr_sol() is called when a NS arrives (line 310).

void gnrc_ipv6_nib_handle_pkt(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
                              const icmpv6_hdr_t *icmpv6, size_t icmpv6_len)
{
    ...
    switch (icmpv6->type) {
        ...
        case ICMPV6_NBR_SOL:
            _handle_nbr_sol(netif, ipv6, (ndp_nbr_sol_t *)icmpv6, icmpv6_len);
            break;
        ...
    }
    ...
}

Second, _copy_and_handle_aro() is called (line 994).

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)
{
   ...
    if (ipv6_addr_is_unspecified(&ipv6->src)) {
        ...
    }
    else {
        ...
        FOREACH_OPT(nbr_sol, opt, tmp_len) {
            switch (opt->type) {
                case NDP_OPT_SL2A:
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LR)
                    if (gnrc_netif_is_6lr(netif)) {
                        DEBUG("nib: Storing SL2AO for later handling\n");
                        sl2ao = opt;
                        break;
                    }
#endif  /* CONFIG_GNRC_IPV6_NIB_6LR */
                    _handle_sl2ao(netif, ipv6, (const icmpv6_hdr_t *)nbr_sol,
                                  opt);
                    break;
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LR)
                case NDP_OPT_AR:
                    DEBUG("nib: Storing ARO for later handling\n");
                    aro = (sixlowpan_nd_opt_ar_t *)opt;
                    break;
#endif  /* CONFIG_GNRC_IPV6_NIB_6LR */
                default:
                    DEBUG("nib: Ignoring unrecognized option type %u for NS\n",
                          opt->type);
                    break;
            }
        }
        reply_aro = _copy_and_handle_aro(netif, ipv6, nbr_sol, aro, sl2ao);
        ...
        if (netif->ipv6.addrs_flags[tgt_idx] & GNRC_NETIF_IPV6_ADDRS_FLAGS_ANYCAST) {
            ...
        }
        else {
            gnrc_ndp_nbr_adv_send(&nbr_sol->tgt, netif, &ipv6->src,
                                  ipv6_addr_is_multicast(&ipv6->dst),
                                  reply_aro);
        }
    }
}

That's where the issue lies. While the if (aro != NULL) branch evaluates as True, _handle_aro() shall return a status of _ADDR_REG_STATUS_IGNORE but this case is not handled and hence _copy_and_handle_aro() returns a reply_aro of NULL. Besides that, it's also weird that a cache full status code does not seem to exist and the same issue arises when a new entry does need to be created but it isn't because the cache is full. In that case the debug statement even explicitly says that no NA is sent in response but that does in fact happen, albeit without an ARO, again.

gnrc_pktsnip_t *_copy_and_handle_aro(gnrc_netif_t *netif,
                                     const ipv6_hdr_t *ipv6,
                                     const ndp_nbr_sol_t *nbr_sol,
                                     const sixlowpan_nd_opt_ar_t *aro,
                                     const ndp_opt_t *sl2ao)
{
    gnrc_pktsnip_t *reply_aro = NULL;

    if (aro != NULL) {
        uint8_t status = _handle_aro(netif, ipv6, (icmpv6_hdr_t *)nbr_sol, aro,
                                     sl2ao, NULL);

        if ((status != _ADDR_REG_STATUS_TENTATIVE) &&
            (status != _ADDR_REG_STATUS_IGNORE)) {
            reply_aro = gnrc_sixlowpan_nd_opt_ar_build(status,
                                                       byteorder_ntohs(aro->ltime),
                                                       (eui64_t *)&aro->eui64,
                                                       NULL);
            if (reply_aro == NULL) {
                DEBUG("nib: No space left in packet buffer. Not replying NS");
            }
        }
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_MULTIHOP_DAD)
        else if (status != _ADDR_REG_STATUS_IGNORE) {
            DEBUG("nib: Address was marked TENTATIVE => not replying NS, "
                  "waiting for DAC\n");
        }
#endif  /* CONFIG_GNRC_IPV6_NIB_MULTIHOP_DAD */
    }
    return reply_aro;
}

For the rest of the flow, see following steps (in order)
line 159 of _nib-6ln.c
line 129 of _nib-6lr.c
line 1000 of nib.c

@miri64 miri64 added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jan 27, 2021
@miri64 miri64 self-assigned this Jan 27, 2021
@miri64
Copy link
Member

miri64 commented Jan 27, 2021

Some initial response without tracking the code you posted in detail for now:

Instead, it seems that a NA is returned, but without an ARO. This behavior does not make sense in any case. If anything, when returning a NA in this case, it should at least contain an ARO with Status code 0, or a NA should not be sent in response at all.

I agree with that. This seems to be a bug.

Besides that, it's also weird that a cache full status code does not seem to exist and the same issue arises when a new entry does need to be created but it isn't because the cache is full.

Yes, it does. It is handled here

nce = _nib_onl_get(&ipv6->src, netif->pid);
/* NIB is full */
if (nce == NULL) {
return SIXLOWPAN_ND_STATUS_NC_FULL;
}

and here

else {
DEBUG("nib: Could not register %s, neighbor cache was full\n",
ipv6_addr_to_str(addr_str, &nce->ipv6, sizeof(addr_str)));
return SIXLOWPAN_ND_STATUS_NC_FULL;
}

The states provided here

/**
* @brief Additional (local) status to ARO status values for tentative
* addresses
*/
#define _ADDR_REG_STATUS_TENTATIVE (3)
/**
* @brief Additional (local) status to ARO status values for return values
* to signify that the address was ignored
*/
#define _ADDR_REG_STATUS_IGNORE (4)
/**
* @brief Additional (local) status to ARO status values to signify that ARO
* is not available in NA
*
* Can be assigned to the variable that receives the return value of
* @ref _handle_aro(), so that the case that an NA does not contain an ARO
* (e.g. because upstream router does not support it) can be dealt with.
*/
#define _ADDR_REG_STATUS_UNAVAIL (255)

are mostly to provide additional states to the once that can be carried within the ARO, when handling the requesting ARO.

If you have a proposal for a fix, feel free to provide a PR.

@tinstructor
Copy link
Author

tinstructor commented Jan 27, 2021

@miri64 I initially wrote this bug report for the first case (which I looked into more extensively) and just assumed this was also a problem for the second case (which it seems is not true). My apologies for jumping to conclusions. Concerning the proposal for a fix, I think the way forward is up for interpretation, as RFC 6775 does not explicitly mention this case (i.e., no pre-existing NCE and an incoming ARO with Registration Lifetime of 0) and / or is vague at best.

For example, RFC 6775 Section 6.5.3. says:

If the ARO contains a zero Registration Lifetime, then any existing NCE for the IPv6 source address of the NS MUST be deleted and an NA sent

However, this has an ambiguous implicit meaning at best for the case wherein there is no pre-existing NCE. Do we ignore the NS altogether? Or do we simply return a NA with Status code = 0? And what would happen then in case of multi-hop DAD?

For example, RFC 6775 Section 8.2.3. says:

When a 6LR receives an NS from a host with a zero Registration Lifetime, then, in addition to removing the NCE for the host as specified in Section 6, a DAR is sent to the 6LBRs as above.

If you opt to return a NA, it does not make sense to also send a DAR to the 6LBR, which would conceivably ignore this DAR anyway since there is no pre-existing entry in the 6LBR DAD-table. In other words, this would be redundant.

Actually, sending a DAR in this case could make sense in case a node wants to know for some reason if an address is already in use but doesn't wish to register it. Then the 6LBR would return a duplicate address status code to the 6LR. However, this would only work if this case was handled synchronously (which does not really make sense if you look at RFC 6775) and the 6LR creates a Tentative NCE, even when the Registration Lifetime is zero. However, RFC 6775 Section 8.2. explicitly says:

When a 6LR receives an NS containing an ARO with a non-zero Registration Lifetime and it has no existing Registered NCE, then with this mechanism the 6LR will invoke synchronous multihop DAD.

From which I'd conclude that our case would be handled asynchronously anyway

@miri64
Copy link
Member

miri64 commented Jan 27, 2021

[…] RFC 6775 does not explicitly mention this case (i.e., no pre-existing NCE and an incoming ARO with Registration Lifetime of 0) and / or is vague at best.

Yeah RFC 6775 is that often (it says e.g. also to send DARs directly to the 6LBR, but does not tell how to know the 6LBRs address; one could suspect to take the address from the ABRO, but as the ABRO is part of the optional multihop prefix dissemination we can not rely on always having that for the equally optional multihop address registration).

If the ARO contains a zero Registration Lifetime, then any existing NCE for the IPv6 source address of the NS MUST be deleted and an NA sent

However, this has an ambiguous implicit meaning at best for the case wherein there is no pre-existing NCE. Do we ignore the NS altogether? Or do we simply return a NA with Status code = 0? […]

I'd say sending status code 0 is the right way to go. The implicit meaning of registration lifetime 0 is the sender of the NS saying "please delete my entry", so there should be a reply to that that the deletion was successful. If there is no pre-existing NCE, this would also mean to signal success, as a deletion of a non-existing item is still a successful deletion ;-).

If you opt to return a NA, it does not make sense to also send a DAR to the 6LBR, which would conceivably ignore this DAR or return a duplicate address code anyway since there is either nor pre-existing entry in the 6LBR DAD-table or the supplied EUI-64 wouldn't match the one stored in any pre-existing entry anyway. In other words, this would be redundant.

If the node asks for the deletion of the entry it should also be deleted at the border router. So a DAR should be send to the 6LBR and a DAD signaling success returned, whether the DAD-table entry existed or not.

Actually, sending a DAR in this case could make sense in case a node wants to know for some reason if an address is already in use but doesn't wish to register it. Then the 6LBR would return a duplicate address status code to the 6LR. However, this would only work if this case was handled synchronously (which does not really make sense if you look at RFC 6775) and the 6LR creates a Tentative NCE, even when the Registration Lifetime is zero. However, RFC 6775 Section 8.2. explicitly says:

When a 6LR receives an NS containing an ARO with a non-zero Registration Lifetime and it has no existing Registered NCE, then with this mechanism the 6LR will invoke synchronous multihop DAD.

From which I'd conclude that our case would be handled asynchronously anyway

This is indeed confusing :-/

@tinstructor
Copy link
Author

tinstructor commented Jan 27, 2021

Yeah RFC 6775 is that often (it says e.g. also to send DARs directly to the 6LBR, but does not tell how to know the 6LBRs address; one could suspect to take the address from the ABRO, but as the ABRO is part of the optional multihop prefix dissemination we can not rely on always having that for the equally optional multihop address registration).

EXACTLY! What I do often is look at how you guys implemented it, or just look at RFC 8505 for how a they handle it there and then devise some in-between solution that is technically RFC 6775 compliant. Concerning the 6LBR addresses, it does indeed say that multi-hop context & prefix distribution is optional but can only be left out if the same functionality is provided by an out-of-scope mechanism. So supposedly, the 6LBR addresses are always available in some data structure that abstracts how they where obtained?

@miri64
Copy link
Member

miri64 commented Jan 27, 2021

EXACTLY! What I do often is look at how you guys implemented it, or just look at RFC 8505 for how a they handle it there and then devise some in-between solution that is technically RFC 6775 compliant.

RFC 8505 did not exist when I did this implementation, so if RFC 8505 provides a solution I would rather suggest to go with that.

@tinstructor
Copy link
Author

@miri64 At first glance it doesn't really provide any solutions but more careful examination is required.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@maribu
Copy link
Member

maribu commented May 17, 2023

@miri64 Could you take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

4 participants