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

Bug in ACD implementation RFC5227 #80429

Closed
anhuba opened this issue Oct 25, 2024 · 2 comments · Fixed by #80433
Closed

Bug in ACD implementation RFC5227 #80429

anhuba opened this issue Oct 25, 2024 · 2 comments · Fixed by #80433
Assignees
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug

Comments

@anhuba
Copy link
Contributor

anhuba commented Oct 25, 2024

The ACD is not properly implemented as described in RFC5227 ch. 2.1.1
The implementation incorrectly detects an IP conflict, if an ARP request is received for the target IP.
The reason is that the current implementation checks for ARP requests instead of ARP probes.

To reproduce enable NET_IPV4_ACD and use DHCP to get an address. Immediately after address is bound send an arp request for the received IP. The DHCP server may already do that. Then a conflict is detected and a new IP requested and so on...

I fixed it with this change, which correctly checks for ARP probe instead of ARP request for the second condition:

--- zephyr/subsys/net/ip/ipv4_acd.c	2024-10-25 11:19:34.000000000 
+++ zephyr/subsys/net/ip/ipv4_acd.c	2024-10-24 16:00:44.000000000 
@@ -285,21 +285,25 @@
 			continue;
 		}
 
 		ll_addr = net_if_get_link_addr(addr_iface);
 
 		/* RFC 5227, ch. 2.1.1 Probe Details:
-		 * - Sender IP address match OR,
-		 * - Target IP address match with different sender HW address,
+		 * - ARP Request/Reply with Sender IP address match OR,
+		 * - ARP Probe where Target IP address match with different sender HW address,
 		 * indicate a conflict.
+		 * ARP Probe has an all-zero sender IP address
 		 */
+		uint32_t all_zero_ip = 0;
 		if (net_ipv4_addr_cmp_raw(arp_hdr->src_ipaddr,
 					  (uint8_t *)&ifaddr->address.in_addr) ||
 		    (net_ipv4_addr_cmp_raw(arp_hdr->dst_ipaddr,
 					  (uint8_t *)&ifaddr->address.in_addr) &&
-		     memcmp(&arp_hdr->src_hwaddr, ll_addr->addr, ll_addr->len) != 0)) {
+		     (memcmp(&arp_hdr->src_hwaddr, ll_addr->addr, ll_addr->len) != 0) &&
+		     (net_ipv4_addr_cmp_raw(arp_hdr->src_ipaddr,
+		                           (uint8_t *)&all_zero_ip)))) {
 			NET_DBG("Conflict detected from %s for %s",
 				net_sprint_ll_addr((uint8_t *)&arp_hdr->src_hwaddr,
 						   arp_hdr->hwlen),
 				net_sprint_ipv4_addr(&ifaddr->address.in_addr));
 
 			iface->config.ip.ipv4->conflict_cnt++;

@anhuba anhuba added the bug The issue is a bug, or the PR is fixing a bug label Oct 25, 2024
@nordicjm
Copy link
Collaborator

Can you submit a PR? We do not accept pasted patched

@anhuba
Copy link
Contributor Author

anhuba commented Oct 25, 2024

#80433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants