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/nib: consolidate prefix removal code in _nib_offl_remove_prefix() #16729

Merged
merged 1 commit into from Aug 26, 2021

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 13, 2021

Contribution description

There are three places in which a prefix gets removed:

  • lifetime timeout in _handle_pfx_timeout()
  • RA handling with PIO with lifetime = 0 in _remove_prefix()
  • manually calling the nib prefix del shell command resulting in gnrc_ipv6_nib_pl_del()

However, they all do slightly different things. The code in _handle_pfx_timeout() appears to be the most complete, so move it to a common function that can be used by the other cases as well.

The _handle_snd_mc_ra() function in gnrc_ipv6_nib_pl_del() did not do much in informing downstream nodes that the prefix was removed - it would only schedule sending a RA without the prefix, but omission does not mean deprecation.
I replaced it with a new _snd_rtr_advs_drop_pfx() function that sends a RA that includes a PIO with a lifetime of 0 to indicate that the prefix must be removed.
I'm not entirely happy about this as I would rather have the removed prefix (with ltime = 0) included in normal RAs for a grace period (in case this RA gets lost) but that would require some more elaborate changes.

Testing procedure

Issues/PRs references

@benpicco benpicco added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Aug 13, 2021
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Aug 13, 2021
@benpicco benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Aug 13, 2021
@benpicco benpicco force-pushed the _nib_offl_remove_prefix branch 2 times, most recently from 21a195f to b18b59c Compare August 14, 2021 13:27
@miri64
Copy link
Member

miri64 commented Aug 24, 2021

Did you test this with your setup?

@benpicco
Copy link
Contributor Author

Yes I ran the test steps from #16750

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Please squash. I trust your testing.

@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 Aug 25, 2021
@benpicco
Copy link
Contributor Author

All green here 😉

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Then lets gooooo 🚀

@miri64 miri64 merged commit eace949 into RIOT-OS:master Aug 26, 2021
@benpicco benpicco deleted the _nib_offl_remove_prefix branch August 26, 2021 17:20
@benpicco
Copy link
Contributor Author

benpicco commented Sep 14, 2021

@miri64 as @chrysn mentioned prefix deprecation, how would this be done in this context?
We have _nib_offl_remove_prefix() but no _nib_offl_deprecate_prefix().
There is no deprecation flag in the _nib_offl_entry_t struct, so how would a prefix be marked as deprecated?

@miri64
Copy link
Member

miri64 commented Sep 15, 2021

There is no deprecation flag in the _nib_offl_entry_t struct, so how would a prefix be marked as deprecated?

A prefix is not deprecated, the associated addresses are (see also RFC 4862, section 5.5.4). This happens here

static void _handle_pfx_timeout(_nib_offl_entry_t *pfx)
{
gnrc_netif_t *netif = gnrc_netif_get_by_pid(_nib_onl_get_if(pfx->next_hop));
uint32_t now = evtimer_now_msec();
gnrc_netif_acquire(netif);
if (now >= pfx->valid_until) {
_nib_offl_remove_prefix(pfx);
}
else if (now >= pfx->pref_until) {
for (int i = 0; i < CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF; i++) {
if (ipv6_addr_match_prefix(&netif->ipv6.addrs[i],
&pfx->pfx) >= pfx->pfx_len) {
netif->ipv6.addrs_flags[i] &= ~GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_MASK;
netif->ipv6.addrs_flags[i] |= GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_DEPRECATED;
}
}
_evtimer_add(pfx, GNRC_IPV6_NIB_PFX_TIMEOUT, &pfx->pfx_timeout,
pfx->valid_until - now);
}
gnrc_netif_release(netif);
}

The timer for that is armed here

if (pref_ltime != UINT32_MAX) {
_evtimer_add(dst, GNRC_IPV6_NIB_PFX_TIMEOUT, &dst->pfx_timeout,
pref_ltime);
/* ignore capped of preferred lifetimes from sec to ms conversion */
if (pref_ltime < (UINT32_MAX - 1)) {
/* prevent pref_ltime from becoming UINT32_MAX */
if (((pref_ltime + now) == UINT32_MAX) && (now != 0)) {
pref_ltime++;
}
pref_ltime += now;
}
}

Sure, we could also add a respective flag and deprecation timer for prefixes to implement RFC 4861, section 6.2.7 to the letter. However, deprecation is in the end only relevant for addresses, so why have another mechanism for prefixes (see the Terminology section of RFC 4862 (section 2))?

 deprecated address -  An address assigned to an interface whose use
   is discouraged, but not forbidden.  A deprecated address should no
   longer be used as a source address in new communications, but
   packets sent from or to deprecated addresses are delivered as
   expected.  A deprecated address may continue to be used as a
   source address in communications where switching to a preferred
   address causes hardship to a specific upper-layer activity (e.g.,
   an existing TCP connection).

@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants