net: Choose IP corresponding to the same network

[anhuba]

If we have more than one IP on an interface we should choose the IP corresponding to the same network as the target IP. Otherwise we might not get ARP replies from some devices.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rlubos]

Please use net: arp: For the commit title prefix, it's an ARP-specifc change.

[rlubos]

is_same_subnet would be more descriptive IMO. Also please fix the indentation, it's off the charts.

[rlubos]

Suggested change

	return((addr1 & netmask) == (addr2 & netmask));
	return ((addr1 & netmask) == (addr2 & netmask));

[rlubos]

So if I read this correctly we'll now have a change in behavior in case there's no subnet match with any of the addresses on the interface. I.e. so far we'd choose any valid address as the source for the ARP request, but now we'd send unspec address as a source on subnet mismatch.

I'm generally fine with the proposed change to prefer addresses on the same subnet, but as I said it should be a preference, not a hard requirement. What if we rewrite the function a bit (which hopefully will improve the clarity as well):

static inline struct net_in_addr *if_get_addr(struct net_if *iface,
					      const uint8_t *own_addr,
					      const struct net_in_addr *dst_addr)
{
	struct net_if_ipv4 *ipv4 = iface->config.ip.ipv4;
	struct net_in_addr *best_match = NULL;

	if (!ipv4) {
		return NULL;
	}

	ARRAY_FOR_EACH(ipv4->unicast, i) {
		if (ipv4->unicast[i].ipv4.is_used &&
		    ipv4->unicast[i].ipv4.address.family == NET_AF_INET &&
		    ipv4->unicast[i].ipv4.addr_state == NET_ADDR_PREFERRED) {
			if (own_addr != NULL) {
				/* Specific address requested as own address. */
				if (!net_ipv4_addr_cmp_raw(
						own_addr,
						ipv4->unicast[i].ipv4.address.in_addr.s4_addr)) {
					/* No match, try next. */
					continue;
				}

				/* Got exact match. */
				best_match = &ipv4->unicast[i].ipv4.address.in_addr;
				break;
			}

			/* Prefer address on the same subnet as destination. */
			if (dst_addr != NULL &&
			    is_same_subnet(dst_addr->s_addr,
				           ipv4->unicast[i].ipv4.address.in_addr.s_addr,
				           ipv4->unicast[i].netmask.s_addr)) {
				/* Got address on the same subnet. */
				best_match = &ipv4->unicast[i].ipv4.address.in_addr;
				break;
			}

			/* Fallback to any valid address on hte interface. */
			if (best_match == NULL) {
				best_match = &ipv4->unicast[i].ipv4.address.in_addr;
			}
		}
	}

	return best_match;
}

(please note I haven't tested this)

[rlubos]

We have two address variables now, it's hard to tell which one represents what. I suggest we call them own_addr and dst_addr for more clarity (the ARP code is already pretty complex enough to follow).