gnrc_netif: highest source address scope wins selection #12409
Conversation
miri64
added
Type: enhancement
|
@cgundogan can you test against the RFC spec? |
|
@cgundogan @tcschmidt please be aware that without #12408 the result of the source address selection gets skewed to the first address assigned to the interface [edit]that won the rest of source address selection[/edit] in any case, so the scope contention might not even come through. |
|
@miri64 this one needs rebasing now. |
miri64
force-pushed
the
gnrc_netif/fix/highest-source-scope-wins
branch
from
e95fc80 to
147e161
Compare
|
Rebased to current master. Please make sure to test this also wrt site-local addresses. |
benpicco
added
the
CI: ready for build
|
How would the testing procedure look like / how did you test this? |
I tested it like I described in the testing procedure, I think (sorry I don't remember exactly). I think for a more in-depth test, one needs to find a set of addresses, where all the other rules result in equal ranking but not the scope and use one of them as destination address and the rest is assigned to the interface as potential source addresses. The largest scope then should win. There can only be three scopes: global, site-local and link-local from largest to smallest. |
There was a problem hiding this comment.
@miri64 the code changes look good and make sense with the RFC, that being said I'm having trouble to understand how I should test this, I understand the test cases but to be completely honest I dont understand how to test scopes regarding multicast address. Should I just add them to the group and ping that group?
| @@ -1067,11 +1064,11 @@ static ipv6_addr_t *_src_addr_selection(gnrc_netif_t *netif, | |||
| uint8_t candidate_scope = _get_scope(ptr); | |||
There was a problem hiding this comment.
I can't seem to comment on that line but one line above (old 1066, new 1063) has a comment which I do not understand, why does it assume both link=local?
There was a problem hiding this comment.
🤷♀️ It doesn't even do that in the first place. There is just this comment saying "both link local", but it does not say what "both" is referring to and if this is an assumption or something else. This comment goes back to the original implementation of this function and even there it doesn't to anything with link local etc. It is not in the code I touched so I deem it unrelated, but if you want, I can add a piggy-back commit to remove it.
There was a problem hiding this comment.
It is not in the code I touched so I deem it unrelated, but if you want, I can add a piggy-back commit to remove it.
Yes it is unrelated but since if was under rule 2, It got me confused. You can add the commit or not, as you said it is unrelated.
There is no involvement of multicast here (unless of course you want to use it as a destination address, but you don't have to). The scope values from multicast addresses are just used because they happen to provide a standadized numerical order to scopes (I believe; just re-using what was there from @OlegHahm's implementation). |
If there is no involvement of multicast how can the scopes be different? From what I understand unicast can have either gobal or local scope. So there would be no case for two candidates to have smaller scope but not equal right? either they both have smaller and equal or one have the same scope. |
|
Ok so I have two nodes with link-local and global scope: node1 ifconfignode2 ifconfigThat works correctly. |
There are the deprecated site-local address, which would provide another level of scope. |
|
In RFC 4291: So I guess alltough good, it doesn't need to be supported? |
|
For multicast AFAIK site local is not deprecated and it is the same check (in a function I don't touch in this PR), so no sense in removing it here. |
|
Thanks @miri64, sorry for asking so many questions. I'm got got quite confused between the RFC's. I changed my setup, two nodes with these address: node1: node2: From node2 to node1 link local, site local wins. From node2 to node1 global, global wins. |
|
|
There was a problem hiding this comment.
This is working, but I think not correctly since in my second test both sources should win EDIT this is not working in case 1 they should have the same preference and in case two proper address is selected by chance but Rule 2. See my suggested changes.Rule 2 is not evaluated. (Unless i'm understanding scopes completely the other way around)
- Scope(SA) < Scope(SB) < Scope(D) : prefered fec0::1711:6b10:65fa:8f36
2020-01-10 18:32:46,347 # inet6 addr: fe80::1711:6b10:65fa:8f36 scope: link VAL
2020-01-10 18:32:46,348 # inet6 addr: fec0::1711:6b10:65fa:8f36 scope: site VAL
> ping6 fd00::7b65:3458:ecde:6b82
2020-01-10 18:32:59,171 # ping6 fd00::7b65:3458:ecde:6b82
2020-01-10 18:32:59,174 # finding the best match within the source address candidates
2020-01-10 18:32:59,175 # Checking address: fe80::1711:6b10:65fa:8f36
2020-01-10 18:32:59,177 # winner for rule 2 (smaller scope) found
2020-01-10 18:32:59,178 # winner for rule 3 found
2020-01-10 18:32:59,180 # Checking address: fec0::1711:6b10:65fa:8f36
2020-01-10 18:32:59,181 # winner for rule 2 (smaller scope) found
2020-01-10 18:32:59,182 # winner for rule 3 found
2020-01-10 18:32:59,184 # Winner is: fec0::1711:6b10:65fa:8f36
2020-01-10 18:33:00,164 # finding the best match within the source address candidates
2020-01-10 18:33:00,166 # Checking address: fe80::1711:6b10:65fa:8f36
2020-01-10 18:33:00,178 # winner for rule 2 (smaller scope) found
2020-01-10 18:33:00,179 # winner for rule 3 found
2020-01-10 18:33:00,181 # Checking address: fec0::1711:6b10:65fa:8f36
2020-01-10 18:33:00,183 # winner for rule 2 (smaller scope) found
2020-01-10 18:33:00,184 # winner for rule 3 found
2020-01-10 18:33:00,185 # Winner is: fec0::1711:6b10:65fa:8f36
2020-01-10 18:33:01,172 # finding the best match within the source address candidates
2020-01-10 18:33:01,174 # Checking address: fe80::1711:6b10:65fa:8f36
2020-01-10 18:33:01,175 # winner for rule 2 (smaller scope) found
2020-01-10 18:33:01,176 # winner for rule 3 found
2020-01-10 18:33:01,178 # Checking address: fec0::1711:6b10:65fa:8f36
2020-01-10 18:33:01,180 # winner for rule 2 (smaller scope) found
2020-01-10 18:33:01,180 # winner for rule 3 found
2020-01-10 18:33:01,181 # Winner is: fec0::1711:6b10:65fa:8f36
- Scope(D) < Scope(SA) < Scope(SB): prefered fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:40,730 # inet6 addr: fec0::7b65:3458:ecde:6b82 scope: site VAL
2020-01-10 18:40:40,736 # inet6 addr: fd00::7b65:3458:ecde:6b82 scope: global VAL
2020-01-10 18:40:27,323 # ping6 fe80::1711:6b10:65fa:8f36
2020-01-10 18:40:27,329 # finding the best match within the source address candidates
2020-01-10 18:40:27,333 # Checking address: fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:27,335 # winner for rule 3 found
2020-01-10 18:40:27,339 # Checking address: fd00::7b65:3458:ecde:6b82
2020-01-10 18:40:27,341 # winner for rule 3 found
2020-01-10 18:40:27,345 # Winner is: fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:27,363 # 12 bytes from fe80::1711:6b10:65fa:8f36: icmp_seq=0 ttl=64 rssi=-46 dBm time=31.844 ms
2020-01-10 18:40:28,329 # finding the best match within the source address candidates
2020-01-10 18:40:28,333 # Checking address: fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:28,335 # winner for rule 3 found
2020-01-10 18:40:28,339 # Checking address: fd00::7b65:3458:ecde:6b82
2020-01-10 18:40:28,341 # winner for rule 3 found
2020-01-10 18:40:28,345 # Winner is: fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:28,360 # 12 bytes from fe80::1711:6b10:65fa:8f36: icmp_seq=1 ttl=64 rssi=-46 dBm time=28.661 ms
2020-01-10 18:40:29,329 # finding the best match within the source address candidates
2020-01-10 18:40:29,333 # Checking address: fec0::7b65:3458:ecde:6b82
2020-01-10 18:40:29,335 # winner for rule 3 found
2020-01-10 18:40:29,339 # Checking address: fd00::7b65:3458:ecde:6b82
2020-01-10 18:40:29,341 # winner for rule 3 found
2020-01-10 18:40:29,344 # Winner is: fec0::7b65:3458:ecde:6b82
|
Addressed comments. I now have a knot in my brain 😅. I also added as a separate commit the proper reading of the scope of multicast addresses. |
| @@ -892,7 +892,12 @@ static int _match_to_idx(const gnrc_netif_t *netif, | |||
|
|
|||
| static uint8_t _get_scope(const ipv6_addr_t *addr) | |||
| { | |||
| if (ipv6_addr_is_link_local(addr)) { | |||
| if (ipv6_addr_is_multicast(addr)) { | |||
There was a problem hiding this comment.
I also added as a separate commit the proper reading of the scope of multicast addresses.
It's in a fixup, did you mean it like that or you wanted an actual separate commit?
There was a problem hiding this comment.
I wanted a fixup. Mainly, because it is a bit beyond the original intention of this PR and I wanted to here your opinion first. I can make it its own commit later-on still.
There was a problem hiding this comment.
I think it should be a separate commit. I don't mind including it, its needed for multicast but in a different commit.
sys/net/gnrc/netif/gnrc_netif.c
Outdated
| @@ -1067,7 +1072,16 @@ static ipv6_addr_t *_src_addr_selection(gnrc_netif_t *netif, | |||
| } | |||
| else if (candidate_scope > dst_scope) { | |||
| DEBUG("winner for rule 2 (larger scope) found\n"); | |||
| winner_set[i] += candidate_scope; | |||
There was a problem hiding this comment.
The only issue is the case source(SA) < source(SB) < source(D), I find sources that say that in that case they both have the same preference and others that sat SB should be preferred. The RFC makes me think it should be SA, but with the if (candidate_scope > dst_scope) they will both get the same preference.
if (A < B) {
if( A < D) {
choose B
} else {
choose A
}
}
else {
choose A
}
BTW: its been a while since something so simple has given me sucha headache ahahaha, just 2 ifs
There was a problem hiding this comment.
BTW: its been a while since something so simple has given me sucha headache ahahaha, just 2 ifs
Yeah... but the wording in the RFC could be a little bit more trivial IMHO so no wonder it gives a headache 😅
There was a problem hiding this comment.
Actually I agree with you: the RFC is pretty clear on the case you talked about: It should be SB: if Scope(SA) < Scope(SB) (true in our case) then If Scope(SA) < Scope(D), (also true in our case) pick then prefer SB […].
There was a problem hiding this comment.
(so we need an else case where SB is picked if Scope(SA) < Scope(SB) < Scope(D), but SC, if Scope(SA) < Scope(SB) < Scope(D) < Scope(SC)... I think that makes the scoring a bit more complicated ^^"
There was a problem hiding this comment.
@fjmolinas I think my latest proposal should make the best selection.
Multicast test, just to verify its returned correctly.
|
Rule 2 of the source address algorithm outlined in [RFC6724] states the possible source addresses must also be compared among each other: > Rule 2: Prefer appropriate scope. > If Scope(SA) < Scope(SB): If Scope(SA) < Scope(D), then prefer SB and > otherwise prefer SA. Similarly, if Scope(SB) < Scope(SA): If > Scope(SB) < Scope(D), then prefer SA and otherwise prefer SB. Our current implementation doesn't do that. It just checks if the scope of a possible source is lesser than the scope of the destination (which involves the second "If" in the rule). This fix grants points according to the scope of an address. If the scope matches, they get the highest points, ensuring that the selected source will always be reachable from the destination. [RFC6724]: https://tools.ietf.org/html/rfc6724
The comment exists since the introduction of the [original implementation], but its meaning is unclear and misleading, as the code doesn't do anything with link-local. [original implementation]: RIOT-OS#3561
|
Squashed. |
miri64
force-pushed
the
gnrc_netif/fix/highest-source-scope-wins
branch
from
1fc54f2 to
018e3dd
Compare
|
🎉 Thanks for all testing and helping with the logic foobar of this PR @fjmolinas! |
Contribution description
Rule 2 of the source address algorithm outlined in RFC6724 states the possible source addresses must also be compared among each other:
Our current implementation doesn't do that. It just checks if the scope of a possible source is lesser than the scope of the destination (which involves the second "If" in the rule).
This fix grants points according to the scope of an address. If the scope matches, they get the highest points, ensuring that the selected source will always be reachable from the destination.
Testing procedure
Pinging between different and same scopes should still work (or even work for the first time).
Issues/PRs references
Follow-up to #12404 and #12408.