Skip link local while checking for redundant announcements or queries

[hrzlgnm]

Otherwise in case multiple network interfaces are present, not all interfaces may be queried and thus services will be removed too early.

I was seeing service removals for services shown as online by other queriers like avahi or python zeroconf. And i was able to track it down to this.

[hrzlgnm]

The issue becomes more prominent in a IPv6 link local only setup. As all link local addresses are in the same subnet.

[hrzlgnm]

There is another occasion of a subnet_set in the code at https://github.com/keepsimple1/mdns-sd/blob/main/src/service_daemon.rs#L1204, perhaps it makes sense to use the same approach also while sending unsolicited responses. Please let me know whether the fix is also required there.

[hrzlgnm]

Tested this change in a pre-release version of https://github.com/hrzlgnm/mdns-browser in my setup where the issue was very prominent and driving me crazy. And I could not observe it anymore.

[hrzlgnm]

Perhaps we only need to handle ipv6 link local adresses in special manner, so the subnect optimization for other address kinds still works.

[hrzlgnm]

Let's put it this way:
Each network interface on a device gets a unique link-local address. Even if two interfaces are on the same physical network, their link-local addresses are treated as being on separate links.Therefore, link-local addresses from different interfaces or devices are not considered to be in the same subnet in the traditional sense, even if they are on the same physical network segment.

[keepsimple1]

If I understand, the problem is that link-local addresses (especially for IPv6 as every IPv6 interface has a link-local address) do not belong to the same network segment even though their netmask (i.e. prefix) match.

In other words, we should not use "subnet" to filter out any link-local addresses. But we can (probably should?) still filter regular non-link-local addresses of the same subnet (even though that might not be very common).

From this perspective, I have a different take about the implementation of the fix, and came up with a diff like this: (in both places where ifaddr_subnet was used)

diff --git a/src/service_daemon.rs b/src/service_daemon.rs
index 8ed4c8c..f06100b 100644
--- a/src/service_daemon.rs
+++ b/src/service_daemon.rs
@@ -1205,11 +1205,15 @@ impl Zeroconf {
 
         for (_, intf_sock) in self.intf_socks.iter() {
             let subnet = ifaddr_subnet(&intf_sock.intf.addr);
-            if subnet_set.contains(&subnet) {
-                continue; // no need to send again in the same subnet.
+            if !intf_sock.intf.is_link_local() {
+                if subnet_set.contains(&subnet) {
+                    continue; // no need to send again in the same subnet.
+                }
             }
             if self.broadcast_service_on_intf(info, intf_sock) {
-                subnet_set.insert(subnet);
+                if !intf_sock.intf.is_link_local() {
+                    subnet_set.insert(subnet);
+                }
                 outgoing_addrs.push(intf_sock.intf.ip());
             }
         }
@@ -1400,11 +1404,13 @@ impl Zeroconf {
 
         let mut subnet_set: HashSet<u128> = HashSet::new();
         for (_, intf_sock) in self.intf_socks.iter() {
-            let subnet = ifaddr_subnet(&intf_sock.intf.addr);
-            if subnet_set.contains(&subnet) {
-                continue; // no need to send query the same subnet again.
+            if !intf_sock.intf.is_link_local() {
+                let subnet = ifaddr_subnet(&intf_sock.intf.addr);
+                if subnet_set.contains(&subnet) {
+                    continue; // no need to send query the same subnet again.
+                }
+                subnet_set.insert(subnet);
             }
-            subnet_set.insert(subnet);
             broadcast_dns_on_intf(&out, intf_sock);
         }
     }

Does it make sense? Will it help address your case?

(and thanks for your PR and investigations!)

[hrzlgnm]

If I understand, the problem is that link-local addresses (especially for IPv6 as every IPv6 interface has a link-local address) do not belong to the same network segment even though their netmask (i.e. prefix) match.

In other words, we should not use "subnet" to filter out any link-local addresses. But we can (probably should?) still filter regular non-link-local addresses of the same subnet (even though that might not be very common).

From this perspective, I have a different take about the implementation of the fix, and came up with a diff like this: (in both places where ifaddr_subnet was used)

diff --git a/src/service_daemon.rs b/src/service_daemon.rs
index 8ed4c8c..f06100b 100644
--- a/src/service_daemon.rs
+++ b/src/service_daemon.rs
@@ -1205,11 +1205,15 @@ impl Zeroconf {
 
         for (_, intf_sock) in self.intf_socks.iter() {
             let subnet = ifaddr_subnet(&intf_sock.intf.addr);
-            if subnet_set.contains(&subnet) {
-                continue; // no need to send again in the same subnet.
+            if !intf_sock.intf.is_link_local() {
+                if subnet_set.contains(&subnet) {
+                    continue; // no need to send again in the same subnet.
+                }
             }
             if self.broadcast_service_on_intf(info, intf_sock) {
-                subnet_set.insert(subnet);
+                if !intf_sock.intf.is_link_local() {
+                    subnet_set.insert(subnet);
+                }
                 outgoing_addrs.push(intf_sock.intf.ip());
             }
         }
@@ -1400,11 +1404,13 @@ impl Zeroconf {
 
         let mut subnet_set: HashSet<u128> = HashSet::new();
         for (_, intf_sock) in self.intf_socks.iter() {
-            let subnet = ifaddr_subnet(&intf_sock.intf.addr);
-            if subnet_set.contains(&subnet) {
-                continue; // no need to send query the same subnet again.
+            if !intf_sock.intf.is_link_local() {
+                let subnet = ifaddr_subnet(&intf_sock.intf.addr);
+                if subnet_set.contains(&subnet) {
+                    continue; // no need to send query the same subnet again.
+                }
+                subnet_set.insert(subnet);
             }
-            subnet_set.insert(subnet);
             broadcast_dns_on_intf(&out, intf_sock);
         }
     }

Does it make sense? Will it help address your case?

Yes makes sense, I'll check it out on my branch and test whether this works for me.

(and thanks for your PR and investigations!)

Glad to use the library and happy to contribute to. ❤️

[hrzlgnm]

cargo clippy or rust-analyzer suggested to collapse this if

[hrzlgnm]

@keepsimple1 I've tested my latest change again with another pre-release version of mdns-browser and it works with your suggested changes.
Thanks for your detailed feedback.

[keepsimple1]

Thanks for updating and verifying! Will merge now.