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

nib: some fixes when a router or a prefix is deleted #20329

Merged
merged 4 commits into from
Jun 10, 2024
Merged

nib: some fixes when a router or a prefix is deleted #20329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5d4fc8
ipv6/nib: delete DC entries, referencing a deleted DR
fabian18 Oct 27, 2022
1d3b320
ipv6/nib: _nib_offl_clear() delete offlink entry when mode is not zero
fabian18 Nov 13, 2022
9648000
ipv6/nib: _handle_rtr_timeout() do not blindly delete offlink entry
fabian18 Nov 13, 2022
bb9ca2e
tests/net/gnrc_ipv6_nib: add tests for router and prefix timeout
fabian18 Feb 2, 2024
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
44 changes: 33 additions & 11 deletions sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "net/gnrc/ipv6/nib/nc.h"
#include "net/gnrc/ipv6/nib.h"
#include "net/gnrc/netif/internal.h"
#include "net/ipv6/addr.h"
#include "random.h"

#include "_nib-internal.h"
Expand Down Expand Up @@ -378,6 +379,20 @@ void _nib_drl_remove(_nib_dr_entry_t *nib_dr)
if (nib_dr->next_hop != NULL) {
_evtimer_del(&nib_dr->rtr_timeout);
nib_dr->next_hop->mode &= ~(_DRL);
#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_DC)
/* When removing a router from the Default
Router list, the node MUST update the Destination Cache in such a way
that all entries using the router perform next-hop determination
again rather than continue sending traffic to the (deleted) router.
(https://datatracker.ietf.org/doc/html/rfc4861#section-6.3.5)
*/
_nib_offl_entry_t *dc = NULL;
while ((dc = _nib_offl_iter(dc))) {
if ((dc->mode & _DC) && dc->next_hop == nib_dr->next_hop) {
_nib_dc_remove(dc);
}
}
#endif
_nib_onl_clear(nib_dr->next_hop);
memset(nib_dr, 0, sizeof(_nib_dr_entry_t));
}
Expand Down Expand Up @@ -538,21 +553,28 @@ static inline bool _in_abrs(const _nib_abr_entry_t *abr)

void _nib_offl_clear(_nib_offl_entry_t *dst)
{
if (dst->next_hop != NULL) {
_nib_offl_entry_t *ptr;
for (ptr = _dsts; _in_dsts(ptr); ptr++) {
/* there is another dst pointing to next-hop => only remove dst */
if ((dst != ptr) && (dst->next_hop == ptr->next_hop)) {
break;
if (dst->mode == _EMPTY) {
Copy link
Member

Choose a reason for hiding this comment

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

This changes the semantics from "Clear off link entry" to "Clear off link entry only if removed from every view"... So please give your reasoning for this. Maybe the the function was used wrong somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assume it was not _EMPTY. Simply memsetting it could be fatal.

Copy link
Member

Choose a reason for hiding this comment

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

I'd argue: then don't call this function, but ok, let's go this route. But no guarantee if this introduces new bugs...

if (dst->next_hop != NULL) {
_nib_offl_entry_t *ptr;
for (ptr = _dsts; _in_dsts(ptr); ptr++) {
/* there is another dst pointing to next-hop => only remove dst */
if ((dst != ptr) && (dst->next_hop == ptr->next_hop)) {
break;
}
}
/* we iterated and found no further dst pointing to next-hop */
if (!_in_dsts(ptr)) {
dst->next_hop->mode &= ~(_DST);
_nib_onl_clear(dst->next_hop);
}
}
/* we iterated and found no further dst pointing to next-hop */
if (!_in_dsts(ptr)) {
dst->next_hop->mode &= ~(_DST);
_nib_onl_clear(dst->next_hop);
}
memset(dst, 0, sizeof(_nib_offl_entry_t));
}
else {
DEBUG("nib: offlink entry %s/%u with mode %u not cleared\n",
ipv6_addr_to_str(addr_str, &dst->pfx, sizeof(addr_str)),
dst->pfx_len, dst->mode);
}
}

_nib_offl_entry_t *_nib_offl_iter(const _nib_offl_entry_t *last)
Expand Down
17 changes: 9 additions & 8 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 @@ -532,7 +532,7 @@
icmpv6_len -= (opt->len << 3), \
opt = (ndp_opt_t *)(((uint8_t *)opt) + (opt->len << 3)))

#if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_ROUTER)

Check warning on line 535 in sys/net/gnrc/network_layer/ipv6/nib/nib.c

View workflow job for this annotation

GitHub Actions / static-tests 

full block {} expected in the control structure
static void _handle_rtr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6,
const ndp_rtr_sol_t *rtr_sol, size_t icmpv6_len)
{
Expand Down Expand Up @@ -1458,17 +1458,18 @@
{
if ((router->next_hop != NULL) && (router->next_hop->mode & _DRL)) {
_nib_offl_entry_t *route = NULL;
unsigned iface = _nib_onl_get_if(router->next_hop);
ipv6_addr_t addr = router->next_hop->ipv6;
_nib_onl_entry_t *next_hop = router->next_hop;

_nib_drl_remove(router);
/* also remove all routes to that router */
/* The Router Lifetime applies only to
the router's usefulness as a default router; it
does not apply to information contained in other
message fields or options. Options that need time
limits for their information include their own
lifetime fields.
(https://datatracker.ietf.org/doc/html/rfc4861#section-4.2) */
while ((route = _nib_offl_iter(route))) {
if ((route->next_hop != NULL) &&
(_nib_onl_get_if(route->next_hop) == iface) &&
(ipv6_addr_equal(&route->next_hop->ipv6, &addr))) {
route->mode = _EMPTY;
route->next_hop->mode &= ~_DST;
if (route->next_hop == next_hop) {
_nib_offl_clear(route);
miri64 marked this conversation as resolved.
Show resolved Hide resolved
/* XXX routing protocol gets informed in case NUD
* determines ipv6->src (still in neighbor cache) to be
Expand Down
139 changes: 127 additions & 12 deletions tests/net/gnrc_ipv6_nib/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,11 +817,11 @@
static size_t _set_rtr_adv(const ipv6_addr_t *ipv6_src,
uint8_t ipv6_hl, uint8_t rtr_adv_code,
bool set_rtr_adv_fields,
uint8_t rtr_adv_flags,
uint8_t rtr_adv_flags, uint32_t t_rtr_reachable_ms, uint16_t t_rtr_alive_s,

Check warning on line 820 in tests/net/gnrc_ipv6_nib/main.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
const uint8_t *sl2ao_addr, size_t sl2ao_addr_len,
uint16_t mtu,
const ipv6_addr_t *pfx, unsigned pfx_len,
uint8_t pfx_flags)
uint8_t pfx_flags, uint32_t t_pfx_valid_s, uint32_t t_pfx_pref_s)
{
size_t icmpv6_len = sizeof(ndp_rtr_adv_t);
ndp_rtr_adv_t *rtr_adv = (ndp_rtr_adv_t *)icmpv6;
Expand All @@ -835,8 +835,8 @@
rtr_adv->flags = rtr_adv_flags;
if (set_rtr_adv_fields) {
rtr_adv->cur_hl = _CUR_HL,
rtr_adv->ltime = byteorder_htons(_RTR_LTIME);
rtr_adv->reach_time = byteorder_htonl(_REACH_TIME);
rtr_adv->ltime = byteorder_htons(t_rtr_alive_s);
rtr_adv->reach_time = byteorder_htonl(t_rtr_reachable_ms);
rtr_adv->retrans_timer = byteorder_htonl(_RETRANS_TIMER);
}

Expand All @@ -857,8 +857,8 @@
pio->len = NDP_OPT_PI_LEN;
pio->prefix_len = pfx_len;
pio->flags = pfx_flags;
pio->valid_ltime = byteorder_htonl(_PIO_PFX_LTIME);
pio->pref_ltime = byteorder_htonl(_PIO_PFX_LTIME);
pio->valid_ltime = byteorder_htonl(t_pfx_valid_s);
pio->pref_ltime = byteorder_htonl(t_pfx_pref_s);
ipv6_addr_init_prefix(&pio->prefix, pfx, pfx_len);
icmpv6_len += sizeof(ndp_opt_pi_t);
}
Expand Down Expand Up @@ -923,9 +923,11 @@
void *state = NULL;
size_t icmpv6_len = _set_rtr_adv(&_rem_gb,
NDP_HOP_LIMIT, 0U, true, 0U,
_REACH_TIME, _RTR_LTIME,
_loc_l2, sizeof(_loc_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A);
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
_netif_exp_t exp_netif;

_get_netif_exp(_mock_netif, &exp_netif);
Expand All @@ -946,9 +948,11 @@
void *state = NULL;
size_t icmpv6_len = _set_rtr_adv(&_rem_ll,
194U, 0U, true, 0U,
_REACH_TIME, _RTR_LTIME,
_loc_l2, sizeof(_loc_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A);
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
_netif_exp_t exp_netif;

_get_netif_exp(_mock_netif, &exp_netif);
Expand All @@ -969,9 +973,11 @@
void *state = NULL;
size_t icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 201U, true, 0U,
_REACH_TIME, _RTR_LTIME,
_loc_l2, sizeof(_loc_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A);
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
_netif_exp_t exp_netif;

_get_netif_exp(_mock_netif, &exp_netif);
Expand All @@ -994,9 +1000,11 @@

_get_netif_exp(_mock_netif, &exp_netif);
_set_rtr_adv(&_rem_ll, NDP_HOP_LIMIT, 201U, true, 0U,
_REACH_TIME, _RTR_LTIME,
_loc_l2, sizeof(_loc_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A);
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
gnrc_ipv6_nib_handle_pkt(_mock_netif, ipv6, icmpv6,
sizeof(ndp_rtr_adv_t) - 1);
TEST_ASSERT_MESSAGE(!gnrc_ipv6_nib_nc_iter(0, &state, &nce),
Expand All @@ -1015,9 +1023,11 @@
void *state = NULL;
size_t icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 201U, true, 0U,
_REACH_TIME, _RTR_LTIME,
_loc_l2, sizeof(_loc_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A);
NDP_OPT_PI_FLAGS_L | NDP_OPT_PI_FLAGS_A,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
ndp_opt_t *opt = (ndp_opt_t *)&_buffer[icmpv6_len];
_netif_exp_t exp_netif;

Expand Down Expand Up @@ -1047,10 +1057,11 @@
void *state = NULL;
size_t icmpv6_len = _set_rtr_adv(&_rem_ll, NDP_HOP_LIMIT, 0U,
set_rtr_adv_fields, rtr_adv_flags,
_REACH_TIME, _RTR_LTIME,
(sl2ao) ? _rem_l2 : NULL, sizeof(_rem_l2),
(mtuo) ? 32397U : 0U,
(pio) ? &_loc_gb : NULL, _LOC_GB_PFX_LEN,
pio_flags);
pio_flags, _PIO_PFX_LTIME, _PIO_PFX_LTIME);
const unsigned exp_addr_count = _netif_addr_count(_mock_netif);
_netif_exp_t exp_netif;

Expand Down Expand Up @@ -1279,6 +1290,108 @@
TEST_ASSERT_EQUAL_INT(0, msg_avail());
}

static void test_handle_router_timeout(void)
{
gnrc_ipv6_nib_ft_t route;
gnrc_ipv6_nib_pl_t prefix;
void *state = NULL;
uint32_t t_rtr_alive_s = 5; /* [0, 9000]s, 0: not a default router */
size_t icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 0U, true, 0U,
_REACH_TIME, t_rtr_alive_s,
_rem_l2, sizeof(_rem_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
gnrc_ipv6_nib_handle_pkt(_mock_netif, ipv6, icmpv6, icmpv6_len);
/* check that prefix has been added */
bool pfx = false, rtr = false;
while (gnrc_ipv6_nib_pl_iter(_mock_netif->pid, &state, &prefix)) {
pfx = true;
TEST_ASSERT_MESSAGE(ipv6_addr_match_prefix(&prefix.pfx, &_loc_gb) >= _LOC_GB_PFX_LEN,
"Prefix is not the one from the RA");
}
if (!pfx) {
TEST_ASSERT_MESSAGE(false, "Prefix not added");
}
/* check that router has been added */
state = NULL;
while (gnrc_ipv6_nib_ft_iter(NULL, 0, &state, &route)) {
if (ipv6_addr_is_unspecified(&route.dst)) {
rtr = true;
TEST_ASSERT_MESSAGE(ipv6_addr_equal(&_rem_ll, &route.next_hop),
"Default route is not via RA source");
}
}
if (!rtr) {
TEST_ASSERT_MESSAGE(false, "Default route not added");
}
/* timeout router by RA with lifetime 0 */
icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 0U, true, 0U,
_REACH_TIME, 0,
_rem_l2, sizeof(_rem_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
gnrc_ipv6_nib_handle_pkt(_mock_netif, ipv6, icmpv6, icmpv6_len);
/* check that default router has been timed out */
state = NULL;
while (gnrc_ipv6_nib_ft_iter(NULL, 0, &state, &route)) {
if (ipv6_addr_is_unspecified(&route.dst)) {
TEST_ASSERT_MESSAGE(ipv6_addr_equal(&_rem_ll, &route.next_hop),
"Default route was not deleted");
}
}
}

static void test_handle_prefix_timeout(void)
{
gnrc_ipv6_nib_ft_t route;
gnrc_ipv6_nib_pl_t prefix;
void *state = NULL;
uint32_t t_rtr_alive_s = 0; /* [0, 9000]s, 0: not a default router */
size_t icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 0U, true, 0U,
_REACH_TIME, t_rtr_alive_s,
_rem_l2, sizeof(_rem_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L,
_PIO_PFX_LTIME, _PIO_PFX_LTIME);
gnrc_ipv6_nib_handle_pkt(_mock_netif, ipv6, icmpv6, icmpv6_len);
/* check that prefix has been added */
bool pfx = false;
while (gnrc_ipv6_nib_pl_iter(_mock_netif->pid, &state, &prefix)) {
pfx = true;
TEST_ASSERT_MESSAGE(ipv6_addr_match_prefix(&prefix.pfx, &_loc_gb) >= _LOC_GB_PFX_LEN,
"Prefix is not the one from the RA");
}
if (!pfx) {
TEST_ASSERT_MESSAGE(false, "Prefix not added");
}
/* check that router has not been added as default router */
state = NULL;
fabian18 marked this conversation as resolved.
Show resolved Hide resolved
while (gnrc_ipv6_nib_ft_iter(NULL, 0, &state, &route)) {
if (ipv6_addr_is_unspecified(&route.dst)) {
TEST_ASSERT_MESSAGE(false, "Router should not have been added as a default router");
}
}
/* timeout router by RA with lifetime 0 */
icmpv6_len = _set_rtr_adv(&_rem_ll,
NDP_HOP_LIMIT, 0U, true, 0U,
_REACH_TIME, 0,
_rem_l2, sizeof(_rem_l2),
32397U, &_loc_gb, _LOC_GB_PFX_LEN,
NDP_OPT_PI_FLAGS_L,
0, 0);
gnrc_ipv6_nib_handle_pkt(_mock_netif, ipv6, icmpv6, icmpv6_len);
/* check that prefix has been timed out */
state = NULL;
if (gnrc_ipv6_nib_pl_iter(_mock_netif->pid, &state, &prefix)) {
TEST_ASSERT_MESSAGE(false, "Prefix has been timed out but is still in use");
}
}

static Test *tests_gnrc_ipv6_nib(void)
{
EMB_UNIT_TESTFIXTURES(fixtures) {
Expand Down Expand Up @@ -1341,6 +1454,8 @@
* we do not have access to the (internally defined) contexts required
* for it */
new_TestFixture(test_change_rtr_adv_iface),
new_TestFixture(test_handle_router_timeout),
new_TestFixture(test_handle_prefix_timeout),
};

EMB_UNIT_TESTCALLER(tests, _set_up, NULL, fixtures);
Expand Down
Loading