Hi Vladislav

On 7/9/19 10:00 PM, Vladislav Grishenko wrote:
> Hi Petr,
> 
> Regarding 0002-Compare-address-and-interface-index-for-allowed-inte.patch, 
> does it support case with different valid interfaces with the same address?
> For example:
>       eth0 192.168.1.1/24
>       tun0 192.168.1.1./16 (created/destroyed dynamically)

Not tested this specific case, but I think it should be handled
correctly, unlike previous code. Because it now compares also interface
index, it will mark existing entry as found only if interface index also
match. If it does not, new entry is created with correct index instead.
It should work, unlike previous code, it should keep both interface
addresses stored separately.

If tun0 is often destroyed and recreated, number of interfaces records
might grow. That is reason for patch #3, which removes dropped
interfaces after creating new ones.
> 
> Regarding appearance, seems newly added code doesn’t fully follow dnsmasq 
> code style in several places:
> * indentation (should be ident ==2 spaces, 8 spaces == \t)
> * brackets on the same code lines
Ok, I forgot to follow style on 3rd patch. Attached fixed formatting and
removed debug log on interface removal.
> * function args on the next line are not aligned with the first argument
> * prettyprint_addr() result is forcibly ignored with (void) unlike other 
> places
I think that is better to state explicitly return value is not used.
> 
> Best Regards, Vladislav Grishenko
> 
> -----Original Message-----
> From: Dnsmasq-discuss <dnsmasq-discuss-boun...@lists.thekelleys.org.uk> On 
> Behalf Of Petr Mensik
> Sent: Tuesday, July 9, 2019 5:31 PM
> To: dnsmasq-discuss@lists.thekelleys.org.uk
> Subject: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated 
> interfaces.
> 
> Hello Simon and others,
> 
> we have discovered issues with TCP DNS query on dnsmasq, when running in 
> bind-dynamic or bind-interfaces mode. dnsmasq scans automatically new 
> interfaces or do that on new query in second case. However, because used 
> speedup comparing only IP adresses in iface_allowed function, it never gets 
> updated index of an interface.
> 
> In case where named interface is destroyed and created again, that drops TCP 
> queries on that interface. They are checked for incoming interface number. If 
> such number is not found in interfaces list, query is denied.
> 
> Luckily, there was a bug in checking, hiding this problem from usual 
> configuration. If IPv6 address is enabled on the new device, new iface entry 
> would be created, because scope_id of sockaddr_in6 does not match previous. 
> That makes even IPv4 queries succeed.
> 
> Bug on bugzilla [1] is partly private.
> 
> I propose three changes. First is just helper to log what happens with 
> listeners on bind-dynamic configuration.
> 
> Second is the most important. Create new interface every time index changes. 
> Also test address family of incoming TCP query when checking allowed clients.
> 
> Third is cleanup of unused interfaces. On some virtual machines hosts, 
> interfaces may often be created and destroyed. It might have negative effect 
> on walking trough interfaces list. I think listeners should be garbage 
> collected also on bind-interfaces configuration. But for now, release memory 
> for unused interfaces at least for bind-dynamic.
> 
> 1. https://bugzilla.redhat.com/show_bug.cgi?id=1721668
> --
> Petr Menšík
> Software Engineer
> Red Hat, http://www.redhat.com/
> email: pemen...@redhat.com  PGP: 65C6C973
> 

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com  PGP: 65C6C973
From 46a77df93b9e5b04f84a031aede0954c0641fe10 Mon Sep 17 00:00:00 2001
From: Petr Mensik <pemen...@redhat.com>
Date: Tue, 9 Jul 2019 14:05:59 +0200
Subject: [PATCH 3/3] Cleanup interfaces no longer available

Clean addresses and interfaces not found after enumerate. Free unused
records to speed up checking active interfaces and reduce used memory.
---
 src/network.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/network.c b/src/network.c
index f487617..44bb757 100644
--- a/src/network.c
+++ b/src/network.c
@@ -533,7 +533,30 @@ static int iface_allowed_v4(struct in_addr local, int if_index, char *label,
 
   return iface_allowed((struct iface_param *)vparam, if_index, label, &addr, netmask, prefix, 0);
 }
-   
+
+/*
+ * Clean old interfaces no longer found.
+ */
+static void clean_interfaces()
+{
+  struct irec *iface;
+  struct irec **up = &daemon->interfaces;
+
+  for (iface = *up; iface; iface = *up)
+  {
+    if (!iface->found && !iface->done)
+      {
+        *up = iface->next;
+        free(iface->name);
+        free(iface);
+      }
+    else
+      {
+        up = &iface->next;
+      }
+  }
+}
+
 int enumerate_interfaces(int reset)
 {
   static struct addrlist *spare = NULL;
@@ -631,6 +654,7 @@ int enumerate_interfaces(int reset)
 	 in OPT_CLEVERBIND mode, that at listener will just disappear after
 	 a call to enumerate_interfaces, this is checked OK on all calls. */
       struct listener *l, *tmp, **up;
+      int freed = 0;
       
       for (up = &daemon->listeners, l = daemon->listeners; l; l = tmp)
 	{
@@ -660,10 +684,14 @@ int enumerate_interfaces(int reset)
 		close(l->tftpfd);
 	      
 	      free(l);
+	      freed = 1;
 	    }
 	}
+
+      if (freed)
+	clean_interfaces();
     }
-  
+
   errno = errsave;
   spare = param.spare;
     
-- 
2.20.1

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to