gnrc/netif: don't treat link local differently in prefix matching

[kaspar030]

Contribution description

RFC6725 describes the algorithm for source address selection.
Rule 9 does not state that link-local address should be treated differently.

Previously, GNRC's _match_to_idx() would always chose the first link-local address, instead of doing a prefix match, in case it was comparing a link-local address.

This PR removes any special handling of link-local addresses from _match_to_idx().

Testing procedure

Test the shit out of GNRC IP.

Issues/PRs references

We thing this happened at some point in #11818.

[miri64]

Since @kaspar030, @fjmolinas, and I discussed this over lunch, I did some digging in the past of this check myself to figure out where this was coming from. It was introduced in 0b80c7e (part of #7370), however the original version in the old gnrc_netif interface did not have this:

RIOT/sys/net/gnrc/network_layer/ipv6/netif/gnrc_ipv6_netif.c

Lines 466 to 480 in 8d941d5

	match = ipv6_addr_match_prefix(&(iface->addrs[i].addr), addr);
	if ((only == NULL) && !ipv6_addr_is_multicast(addr) &&
	(match < iface->addrs[i].prefix_len)) {
	/* match but not of same subnet */
	continue;
	}
	if (match > best_match) {
	if (res != NULL) {
	*res = &(iface->addrs[i].addr);
	}
	best_match = match;
	}

(the new gnrc_netif doesn't store multicast and unicast addresses in the same array anymore, so the multicast check performed there is not necessary anymore). So I don't know why I added this line... The (match < 64) part came from a misguided attempt to say "link-local prefixes are always of 64-bit length" to be in line with RFC 4291, I guess. But I'm not sure...

I also asked @bergzand and @cgundogan (who reviewed #7370) but they didn't know either.

As such, I'm willing to ACK this, I just want to run some tests first.

[miri64]

tests/gnrc_netif fails on native with this:

..................
tests.test_ipv6_addr_match__wrong_addr (tests/gnrc_netif/main.c 369) exp -1 was 0
...................................................
run 69 failures 1

[waehlisch]

@miri64, i don't have time to check the code now but what you have to ensure is that the set of candidate source addresses should only include link-local addresses if the destination address is a link-local address. from this point of view, link-local addresses are treated differently compared to global addresses.

[miri64]

@miri64, i don't have time to check the code now but what you have to ensure is that the set of candidate source addresses should only include link-local addresses if the destination address is a link-local address. from this point of view, link-local addresses are treated differently compared to global addresses.

The check @kaspar030 removes is applied in Rule 8 of the source address selection, so long after the candidate set was generated. I'm investigating at the moment why the test fails. This could give the answer why the check is there for matching prefixes (for a different use case other than source address selection), but I'm still investigating. From what I know so far: In the scope of source address selection I am however pretty sure by now, that the check removed here was invalid indeed.

[miri64]

(the current implementation apart from some tweaks on my side goes back to an implementation @OlegHahm did for an earlier version of GNRC btw, so for me it is also "other peoples code" ;-))

[miri64]

The (match < 64) part came from a misguided attempt to say "link-local prefixes are always of 64-bit length" to be in line with RFC 4291, I guess. But I'm not sure...

The failing test at least confirms this suspicion, not sure if it is justified though.

[miri64]

I tested the following now:

1. tests/gnrc_netif ✔️
2. link-local ping between two examples/gnrc_networking native instances ✔️
3. pinging my host machine via a examples/gnrc_border_router from a examples/gnrc_networking on samr21-xpro ❌

On master 3. is working. I'm trying to figure out what's wrong...

[miri64]

3. pinging my host machine via a examples/gnrc_border_router from a examples/gnrc_networking on samr21-xpro x

On master 3. is working. I'm trying to figure out what's wrong...

I'm pinging the fd00:dead:beef::1 address uhcpd adds to the host machine. In that case the interface's link-local address is the better match than the global address 2001:db8:: so something seems to be wrong in the candidate set selection also...

[miri64]

3. pinging my host machine via a examples/gnrc_border_router from a examples/gnrc_networking on samr21-xpro x
   On master 3. is working. I'm trying to figure out what's wrong...

I'm pinging the fd00:dead:beef::1 address uhcpd adds to the host machine. In that case the interface's link-local address is the better match than the global address 2001:db8:: so something seems to be wrong in the candidate set selection also...

I believe rule 2 is (and originally was) implemented wrong. It is stated:

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.

If I'm not mistaken this means "If an address has a larger scope, prefer it". The code however adds points for having a smaller scope (which also doesn't make much sense to me from a networking perspective):

RIOT/sys/net/gnrc/netif/gnrc_netif.c

Lines 1037 to 1043 in 22b0873

	else if (candidate_scope < dst_scope) {
	DEBUG("winner for rule 2 (smaller scope) found\n");
	winner_set[i] += RULE_2B_PTS;
	if (winner_set[i] > max_pts) {
	max_pts = winner_set[i];
	}
	}

Also the case in #3561:

RIOT/sys/net/gnrc/network_layer/ipv6/netif/gnrc_ipv6_netif.c

Lines 718 to 724 in 8d941d5

	else if (candidate_scope < dst_scope) {
	DEBUG("winner for rule 2 (smaller scope) found\n");
	winner_set[i] += RULE_2B_PTS;
	if (winner_set[i] > max_pts) {
	max_pts = winner_set[i];
	}
	}

[miri64]

Ah the "smaller" part only applies to the "If Scope(SA) < Scope(D)" bit, as only the candidate's scope and the destination scope are compared. I think of instead using arbitrary points, the scope should be used as points.

[miri64]

In any case: as the PR stands now, it doesn't fix source address selection only, but prefix matching in general. As such I rename it to your commit message.

[waehlisch]

If I'm not mistaken this means "If an address has a larger scope, prefer it". The code however adds points for having a smaller scope (which also doesn't make much sense to me from a networking perspective):

you should select a source address that is reachable (at least from the scope point of view) from the destination address. For example, if the destination address is global but you set a link local source address, a reply to this source address will fail.

[miri64]

The scope selection, while faulty its current form wasn't actually the problem for my issue. The problem was, after all addresses were considered by source selection it then proceeded trying to match the found address to the destination address which for ULAs (prefixed with fd::/7) doesn't work, if the global address starts e.g. with 2001::/8. As I read the RFC, the longest prefix match should only be used as a final tie-breaker if there are more than one possible source addresses left. See #12408 for a fix.

[miri64]

@kaspar030 please squash! I think it makes it easier if I pull the remaining commit in this PR into #12408 so the test case becomes more obvious.

[kaspar030]

I think it makes it easier if I pull the remaining commit in this PR into #12408 so the test case becomes more obvious.

👍

[miri64]

Thanks!

[kaspar030]

Closing in favor of #12408.