nib: some fixes when a router or a prefix is deleted #20329
Conversation
benpicco
added
Type: bug
fabian18
force-pushed
the
nib_rtr_fixes
branch
from
a7d7c3f to
7347679
Compare
fabian18
force-pushed
the
nib_rtr_fixes
branch
from
7347679 to
5638f17
Compare
| /* 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Assume it was not _EMPTY. Simply memsetting it could be fatal.
There was a problem hiding this comment.
I'd argue: then don't call this function, but ok, let's go this route. But no guarantee if this introduces new bugs...
| /** 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. | ||
| @see [RFC4861]https://datatracker.ietf.org/doc/html/rfc4861#section-4.2 */ |
There was a problem hiding this comment.
How does this comment relate to the fix below? Or should it even? Below you just seem to apply pointer arithmetics, which is a nice optimization, but the comparison has nothing to do with RA fields...
There was a problem hiding this comment.
The comment expresses the opposite of the previous comment.
Offlink entries are _PFX, _DC, _FT.
Before, all associated prefixes were deleted. But prefixes have their own lifetime.
The _DC flag is cleared in _nib_drl_remove.
Assume any flag would be set in route->mode. That means it is present in some data structure.
If the mode is just set to _EMPTY and if _nib_offl_clear() simply memsets() the entry, it is not properly deleted.
An offlink entry must be deleted properly with (_nib_pl_remove, _nib_ft_remove, _nib_dc_remove).
I remember when I was debugging at that time I saw that the memset was brutally overwriting present entries.
|
Are all the changes from this PR now merged or is there still something left? |
|
Not all, I am about to rebase this PR |
fabian18
force-pushed
the
nib_rtr_fixes
branch
from
35e1234 to
6ed2fbf
Compare
fabian18
force-pushed
the
nib_rtr_fixes
branch
from
6ed2fbf to
c8412c1
Compare
|
@miri64 have your comments been addressed? |
|
I will have a look at this, and the other GNRC PRs, next week at Hack'n'ACK. |
miri64
dismissed
their stale review
Not agreeing with how the comments were addressed, but not blocking either.
Contribution description
I still had some fixes for NIB lying around.
I remember they were necessary during my thesis.
The fixes are old and probably would need some confirmation, but I think they are still valid.
Testing procedure
I will try to show their necessity by adding something to our tests.
Issues/PRs references