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

gnrc/network_layer/ipv6/nib: fix packet leak with unreachable neighbors #20781

Closed

Conversation

derMihai
Copy link
Contributor

@derMihai derMihai commented Jul 9, 2024

Contribution description

This following fix only applies for CONFIG_GNRC_IPV6_NIB_QUEUE_PKT == 1 (default config).

There is currently the case that a on-link neighbor's packet queue might never be emptied. Specifically: if a neighbor is unreachable, the packets queued to be later send (i.e. once the neighbor is resolved) will never be de-queued if the neighbor is never resolved. On a typical link (e.g. ethernet), this is less of a problem, as the NIB subsystem actively tries to resolve the host, and in case of failure the neighbor entry is removed and the packets in it's queue get released. However, on e.g. 6LoWPANs, there is no active discovery going on, so the code path for removing the neighbor never gets executed.

This PR adds a new event that flushes the packets in a neighbor's queue if and only if the neighbor is unreachable.
The flushing event is enqueued only when one of the following events happen:

  • the neighbor is unreachable, it's packet queue is empty, and a packet is enqueued in it's queue
  • the neighbor switches status from reachable to unreachable and it's packet queue is not empty

Additionally, a neighbor's packet queue is now capped at CONFIG_GNRC_IPV6_NIB_QUEUE_PKT_CAP. If this limit is reached before the flushing event is triggered, the oldest packets will get discarded. This is to ensure that packet-flooding a neighbor won't cause a DoS on others, should the packet pool get depleted before the flushing event.

Testing procedure

Here is some example code that triggers the bug:

void udp_send(void)
{
    sock_udp_t sock;
    sock_udp_ep_t local;
    sock_udp_ep_t remote;

    int res = sock_udp_str2ep(&local, "[fdf7:0:0:ff::1]:777");
    assert(res == 0);
    res = sock_udp_str2ep(&remote, "[fdf7:0:0:ff::ff]:777");
    assert(res == 0);

    res = sock_udp_create(&sock, &local, NULL, 0);
    assert(res == 0);

    static char const to_send[] = "I receive, you receive <>";

    while (1) {
        res = sock_udp_send(&sock, to_send, sizeof(to_send), &remote);
        (void) to_send;
        if (res < 0) {
            // once all the packets in the queue pool get used up, it fails with -ENOMEM
            printf("%s: send error: %d\n", __func__, res);
        } else {
            // this works for a while
            printf("%s: sent %d bytes\n", __func__, res);
        }
        ztimer_sleep(ZTIMER_MSEC, 500);
    }
    return NULL;
}

static void config_routes(void)
{
    gnrc_netif_t const *rf_itf = gnrc_netif_get_by_type(NETDEV_AT86RF215, NETDEV_INDEX_ANY);
    assert(rf_itf != NULL);

    ipv6_addr_t subnet;
    ipv6_addr_t *res = ipv6_addr_from_str(&subnet, "fdf7::");
    assert(res != NULL);

    ipv6_addr_t gateway;
    res = ipv6_addr_from_str(&gateway, "fdf7:0:0:ff::ff");
    assert(res != NULL);

    // add a static route, which automatically adds a neighbor entry that never gets resolved
    int resi = gnrc_ipv6_nib_ft_add(&subnet, 64, &gateway, rf_itf->pid, 0);
    assert(resi == 0);

    // if this gets compiled in, the bug is not triggered anymore as the neighbor is then marked as
    // unmanaged and does not count as unresolved
    // uint8_t const l2_addr[] = { 0xE6, 0x41, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6 };
    // resi = gnrc_ipv6_nib_nc_set(&gateway, rf_itf->pid, l2_addr, sizeof(l2_addr));
    // assert(resi == 0);
}

static void config_addresses(void)
{
    gnrc_netif_t *itf = gnrc_netif_get_by_type(NETDEV_AT86RF215, NETDEV_INDEX_ANY);
    assert(itf);

    int res = gnrc_netif_ipv6_addr_add(itf, "fdf7:0:0:ff::1", 64, 0);
    assert(res >= 0);
}

int main(void)
{
    int ret;
    // 8< 
    config_addresses();
    config_routes();
    udp_send();
   // 8< 
}

I briefly tested the changes with both NETDEV_SAM0_ETH and NETDEV_AT86RF215. However, please keep in mind that this is my first time digging in the RIOT network stack, so there might be some edge-cases that I might have missed.

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 9, 2024
@derMihai derMihai force-pushed the mir/upstream/fix_nib_packet_leak branch from be0bfd0 to 32dad42 Compare July 9, 2024 13:19
@benpicco benpicco requested a review from fabian18 July 11, 2024 08:34
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 11, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 26, 2024
@riot-ci
Copy link

riot-ci commented Jul 26, 2024

Murdock results

✔️ PASSED

7fcc59c sys/net/gnrc/network_layer/ipv6/nib: fix packet leak with unreachable neighbors

Success Failures Total Runtime
10196 0 10196 15m:17s

Artifacts

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks very sensible to me, maybe @miri64 wants to give this a look, otherwise this is good to merge IMHO (sans small comments)

@miri64
Copy link
Member

miri64 commented Jul 26, 2024

LGTM, but only scanned the code and did not test anything. If you tested this @benpicco, feel free to ACK.

@derMihai derMihai force-pushed the mir/upstream/fix_nib_packet_leak branch 3 times, most recently from 0617544 to c9d79dd Compare July 30, 2024 06:02
@derMihai derMihai marked this pull request as draft July 30, 2024 10:29
@derMihai derMihai force-pushed the mir/upstream/fix_nib_packet_leak branch from c9d79dd to 9340100 Compare July 30, 2024 10:32
@derMihai derMihai marked this pull request as ready for review July 30, 2024 10:36
if (node->pktqueue_len == CONFIG_GNRC_IPV6_NIB_QUEUE_PKT_CAP) {
gnrc_pktqueue_t *oldest = _nbr_pop_pkt(node);
assert(oldest != NULL);
gnrc_pktbuf_release_error(oldest->pkt, EHOSTUNREACH);
Copy link
Contributor

Choose a reason for hiding this comment

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

So far, this always comes together with:
gnrc_icmpv6_error_dst_unr_send(ICMPV6_ERROR_DST_UNR_ADDR, oldest->pkt);
before in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, fixed!

_evtimer_add(nce, GNRC_IPV6_NIB_FLUSH_PCK_QUEUE,
&nce->flush_queue_timeout,
CONFIG_GNRC_IPV6_NIB_QUEUE_PKT_LINGER_MS);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but think about it:
If the neighbor becomes reachable again suddenly, should the timer to flush the queue be canceled.

I guess,

void _nbr_flush_pktqueue(_nib_onl_entry_t *node)
{
    if (_is_reachable(node)) {
        return;
    }
[...]

is supposed to catch that case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. There is no common entry point for setting a neighbor reachable, so that we can cancel the the timer there. Also _is_reachable() == true doesn't require the neighbor to be in REACHABLE state, but to be neither in INCOMPLETE nor UNREACHABLE. We probably don't want to discard the packets in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regarding memory safety, the timer is canceled before the neighbor entry goes out of scope anyway.

@fabian18
Copy link
Contributor

fabian18 commented Jul 31, 2024

Informational: the UNREACHABLE state is an extension.
RFC7048

Maybe it is worth reading it quickly (it is short) as it is related to the topic.

@fabian18
Copy link
Contributor

What is the packet linger (5ms)?

@derMihai
Copy link
Contributor Author

derMihai commented Aug 1, 2024

What is the packet linger (5ms)?

it's the timeout for packets waiting in the queue of an unreachable neighbor.

@derMihai
Copy link
Contributor Author

derMihai commented Aug 1, 2024

Informational: the UNREACHABLE state is an extension. RFC7048

Maybe it is worth reading it quickly (it is short) as it is related to the topic.

Yes, this document is specific for the UNREACHABLE state. But I am dropping the packets also in in he INCOMPLETE state (i.e. _is_reachable() returns false). If I understand the NIB code correctly, in 6lo the UNREACHABLE state is never set, if the neighbor was never in state REACHABLE, as this goes through NS only by following state transition: REACHABLE -> STALE -> DELAY -> PROBE. Only once PROBE is reached, _probe_nbr() is called periodically, which sends UC NS, which can transition to UNREACHABLE. And indeed, I could observe this exact behavior.

I wonder whether this is a bug, and we need a way to get to UNREACHABLE from any state. Once UNREACHABLE is set, then flush all packets. We don't need the timer for the flushing event anymore in this case, as this implies a delay.

@derMihai derMihai force-pushed the mir/upstream/fix_nib_packet_leak branch from 9340100 to 7fcc59c Compare August 1, 2024 09:20
@benpicco
Copy link
Contributor

benpicco commented Aug 1, 2024

If we never do multicast address resolution in 6lo, couldn't _resolve_addr() be then simplified as

--- a/sys/net/gnrc/network_layer/ipv6/nib/nib.c
+++ b/sys/net/gnrc/network_layer/ipv6/nib/nib.c
@@ -1374,9 +1374,12 @@ static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
     }
 
     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) {
+        /* don't do multicast address resolution on 6lo */
+        if (gnrc_netif_is_6ln(netif)) {
+            return false;
+        }
+
         entry = _nib_nc_add(dst, netif ? netif->pid : 0,
                             GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
         if (entry == NULL) {
@@ -1404,10 +1407,9 @@ static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
         return false;
     }
 
-    /* don't do multicast address resolution on 6lo */
-    if (!gnrc_netif_is_6ln(netif)) {
-        _probe_nbr(entry, reset);
-    }
+    DEBUG("nib: resolve address %s by probing neighbors\n",
+          ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
+    _probe_nbr(entry, reset);
 
     return false;
 }

@derMihai
Copy link
Contributor Author

derMihai commented Aug 2, 2024

If we never do multicast address resolution in 6lo, couldn't _resolve_addr() be then simplified as

--- a/sys/net/gnrc/network_layer/ipv6/nib/nib.c
+++ b/sys/net/gnrc/network_layer/ipv6/nib/nib.c
@@ -1374,9 +1374,12 @@ static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
     }
 
     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) {
+        /* don't do multicast address resolution on 6lo */
+        if (gnrc_netif_is_6ln(netif)) {
+            return false;
+        }
+
         entry = _nib_nc_add(dst, netif ? netif->pid : 0,
                             GNRC_IPV6_NIB_NC_INFO_NUD_STATE_INCOMPLETE);
         if (entry == NULL) {
@@ -1404,10 +1407,9 @@ static bool _resolve_addr(const ipv6_addr_t *dst, gnrc_netif_t *netif,
         return false;
     }
 
-    /* don't do multicast address resolution on 6lo */
-    if (!gnrc_netif_is_6ln(netif)) {
-        _probe_nbr(entry, reset);
-    }
+    DEBUG("nib: resolve address %s by probing neighbors\n",
+          ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
+    _probe_nbr(entry, reset);
 
     return false;
 }

no, because you still want it resolved. i.e. if the neighbor is a router, it can be resolved with a router advertisement. See the _enqueue_for_resolve() further below in the function.

@fabian18
Copy link
Contributor

fabian18 commented Aug 6, 2024

I am not so much convinced about the necessary complexity of the new timeout to release pending packets.

I would have just moved your _nbr_flush_pktqueue to:

if (state == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE) {
/* first 3 retransmissions in PROBE, assume 1 higher to
* not send after netif->ipv6.retrans_timer sec again,
* but the next backoff after that => subtract 2 */
retrans_time = _exp_backoff_retrans_timer(nbr->ns_sent - 2,
retrans_time);
}

Each time the NUD state of a neighbor becomes UNREACHABLE as a result of probing, all pending packets are flushed. The exponential backoff is capped at 60s. So at latest after 60s all pending packets are deleted.

@derMihai
Copy link
Contributor Author

derMihai commented Aug 6, 2024

I am not so much convinced about the necessary complexity of the new timeout to release pending packets.

I would have just moved your _nbr_flush_pktqueue to:

if (state == GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE) {
/* first 3 retransmissions in PROBE, assume 1 higher to
* not send after netif->ipv6.retrans_timer sec again,
* but the next backoff after that => subtract 2 */
retrans_time = _exp_backoff_retrans_timer(nbr->ns_sent - 2,
retrans_time);
}

Each time the NUD state of a neighbor becomes UNREACHABLE as a result of probing, all pending packets are flushed. The exponential backoff is capped at 60s. So at latest after 60s all pending packets are deleted.

which is exactly what I suggested above, because UREACHABLE already implies a delay. Thing is, the UNREACHABLE state is never reached on 6lo (see above). Also, 60s is IMO too long, you can run out of free packets way before that.

@benpicco
Copy link
Contributor

benpicco commented Aug 6, 2024

if the neighbor is a router, it can be resolved with a router advertisement.

but that's purely by accident, right?
AFAIU there is nothing in the code that would trigger a router advertisement during address resolution.

@derMihai
Copy link
Contributor Author

derMihai commented Aug 6, 2024

AFAIU there is nothing in the code that would trigger a router advertisement during address resolution.

As stated in https://www.rfc-editor.org/rfc/rfc6775.html#section-3.3 and by observation, we do. At some point he host sends a router solicitation. During resolution, _enqueue_for_resolve() appends the packet in the neighbor's (router's) queue instead of dropping them.

@fabian18
Copy link
Contributor

fabian18 commented Aug 6, 2024

Thing is, the UNREACHABLE state is never reached on 6lo (see above)

I thought the PR has no impact on 6lowpan, because packets are not queued there.

5.6 of RFC 6775

@fabian18
Copy link
Contributor

fabian18 commented Aug 6, 2024

Thing is, the UNREACHABLE state is never reached on 6lo (see above)

I thought the PR has no impact on 6lowpan, because packets are not queued there.

5.6 of RFC 6775

A LoWPAN node is not required to maintain a minimum of one buffer per
   neighbor as specified in [[RFC4861](https://www.rfc-editor.org/rfc/rfc4861)], since packets are never queued
   while waiting for address resolution.

@derMihai
Copy link
Contributor Author

derMihai commented Aug 6, 2024

   neighbor as specified in [[RFC4861](https://www.rfc-editor.org/rfc/rfc4861)], since packets are never queued
   while waiting for address resolution.

Interesting. They were queued the whole time, this is where this whole PR started from. Then I guess @benpicco 's suggestion is once again valid: just drop packets. For any other links, we might just drop all the packets in the queue once the neighbor's status switches to UNREACHABLE, and that should happen fast (3s IRC). The only thing that has to be made sure is that the UNREACHABLE status can always be reached. In 6lo that is not always the case (see above).

@derMihai
Copy link
Contributor Author

derMihai commented Aug 8, 2024

I did some more digging, on non-6lo: if the STALE state is reached, the UNREACHABLE state will be eventually reached. DELAY and PROBE can be reached only through STALE. So the only question remaining is if the transition INCOMPLETE -> STALE is guaranteed to always happen, and this is where I'm getting overwhelmed by the complexity of the code-base paired with my lack of knowledge in IPv6.

I see two solutions:

Solution 1

Assume INCOMPLETE -> STALE is guaranteed to happen, or maybe someone more experienced can confirm this is true. Then:

  • for 6lo: drop packets, don't enqueue for resolve (@benpicco 's suggestion)
  • for anything else: drop queued packets once in UNREACHEABLE (@fabian18 's suggestion)

Solution 2

Assume nothing and stick with the current solution, but also don't enqueue packets on 6lo.

I'd rather go with solution 1, as It's simpler and cleaner.

@fabian18
Copy link
Contributor

fabian18 commented Aug 9, 2024

So the only question remaining is if the transition INCOMPLETE -> STALE is guaranteed to always happen

I think when you ping an address that does not exist, the NCE will be added as INCOMPLETE but it will timeout and the neighbor will be deleted once the maximum number of probes is reached.
For the STALE state to be reached you need to receive something from a node.

A neighbor entry starts as INCOMPLETE when we first try to talk to him. If he talks to us first, it is added as STALE until it becomes REACHABLE by a replying with a solicited NA, or UNREACHABLE if we want to send something to him but dont get any reachability confirmation.

Maybe you found this image already on google: https://njetwork.wordpress.com/wp-content/uploads/2014/01/msi_ipv6-nd-state-machine1.png

@derMihai
Copy link
Contributor Author

derMihai commented Aug 12, 2024

I think when you ping an address that does not exist, the NCE will be added as INCOMPLETE but it will timeout and the neighbor will be deleted once the maximum number of probes is reached.

yes, that happens in _handle_snd_ns() in _nib-arsm.c. But shouldn't the deletion be skipped if the online entry is part of an offline entry? I see that such entries are marked with dst->next_hop->mode |= _DST; in _nib_offl_alloc(). I assume such entries must not be freed, yet there is nothing ever checking this flag. Have I just found another bug?

@miri64
Copy link
Member

miri64 commented Aug 12, 2024

Maybe you found this image already on google: https://njetwork.wordpress.com/wp-content/uploads/2014/01/msi_ipv6-nd-state-machine1.png

Not sure if I had something similar in there, but back when I implemented the NIB, I drew some diagrams as well https://github.com/miri64/riot-ndp-model/blob/master/index.md

@miri64
Copy link
Member

miri64 commented Aug 12, 2024

(also note, that the 6Lo-ND state machine is slightly different. Since UNREACHABLE is an extension to ND (RFC 7048), and if I interpret RFC 6776, section 13 correctly, it might be more sensible to drop the UNREACHABLE state altogether for 6LNs, and just fall back to original RFC 4861 behavior, i.e., as per the state machine in RFC 4861, appendix A C).

@fabian18
Copy link
Contributor

fabian18 commented Aug 18, 2024

But shouldn't the deletion be skipped if the online entry is part of an offline entry?

Do you really observe a bad/strange situation like this or is this hypothetical?
When I do: nib route add 6 fdf7::/16 fdf7:0:0:ff::ff, fdf7:0:0:ff::ff is added to the onlink entries, but the _NC is not set, so it is not considered to be in the neighbor cache. That's why it does not show up with nib neigh.

When you ping fdf7:0:0:ff::ff it will get the INCOMPLETE state and the _NC flag and _nib_nc_remove is called where you said. The _NC flag is cleared and the the packed queue is emptied.
But at the _nib_onl_clear does nothing because the mode is not _EMPTY but in _DST.

@fabian18
Copy link
Contributor

(also note, that the 6Lo-ND state machine is slightly different. Since UNREACHABLE is an extension to ND (RFC 7048), and if I interpret RFC 6776, section 13 correctly, it might be more sensible to drop the UNREACHABLE state altogether for 6LNs, and just fall back to original RFC 4861 behavior, i.e., as per the state machine in RFC 4861, appendix A C).

Something that RIOT does not do at the moment is searching a new default router, once the current default router becomes UNREACHABLE. At the moment for 6LN if the default router is probed with unicast NS and does no longer respond, it becomes UNREACHABLE and even multicast NS would be sent for 6LN.

In theory there is a list of default routers. (RIOT supports 1 by default).
If one default router does no longer respond you should search for new routers and pick another default router from the default router list, if there are any routers which are probably reachable. If all routers are UNREACHABLE an old router from the list should be dropped in favor of a new router from which we just received an RA from, and is thus probably reachable.

This is out of scope for this PR. The focus here should just be to get rid of the leak.

@derMihai
Copy link
Contributor Author

Do you really observe a bad/strange situation like this or is this hypothetical?

Hypothetical, but I realize now that I missed the detail that in the case of a static route, there are two on-link entries: one bound to the off-link entry, and one in the nc. It is the former on which packets are enqueued.

We are still left with the following two problems:

  1. in 6lo, NS are not sent -> so nc entries in INCOMPLETE state are not released -> queued packets are not released
  2. in 6lo, the transition INCOMPLETE to STALE is never going to happen if REACHABLE is never set (at least what happens for me and I understand from the code) -> UNREACHABLE is never set -> we can't drop packets when switching to UNREACHABLE

So then:

Solution 1

Assume INCOMPLETE -> STALE is guaranteed to happen, or maybe someone more experienced can confirm this is true. >Then:

  • for 6lo: drop packets, don't enqueue for resolve (@benpicco 's suggestion)
  • for anything else: drop queued packets once in UNREACHEABLE (@fabian18 's suggestion)

@derMihai
Copy link
Contributor Author

#20834

@derMihai
Copy link
Contributor Author

Solved with #20834.

@derMihai derMihai closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants