[Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.

Vladislav Grishenko themiron.ru at gmail.com
Wed Jul 10 12:34:57 BST 2019


Hi Petr,

> 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.

Checked, unfortunately interface index comparison breaks the things.
If there's 2+ interface with same address on startup, no error is happen, single TCP and multiple UDP (unlike before) sockets are created, on any of such interface shutdown - thins single TCP socket is closed (unlike before), so there are noting listens on TCP:53 after that.
If there's only one interface on startup - single TCP&UDP sockets are created, on subsequent interface up with the same address - bind error raised and only UDP socket is created additionally (unlike before).

At the moment, dnsmasq logic expects single TCP/UDP socket per address even for multiple interfaces.
For example, comment in iface_allowed() states that:
	/* check whether the interface IP has been added already
	 * we call this routine multiple times */
So, I'm afraid, seems proposed changes does not play well with that.
How do you think, can it be solved too?
Reproduction this case is quite easy, just need to create dummy interface with same address (different netmask) and up/down it.

> Ok, I forgot to follow style on 3rd patch. Attached fixed formatting and removed debug log on interface removal.

Thanks,  fyi sed -r 's/[ ]{8}/\t/' is missed too.

> I think that is better to state explicitly return value is not used.

I think that would be better to rip it off from functional patch, and let it be as separate full patch for all prettyprint_* instances not just for some selected.
At the other hand, with no __attribute__((warn_unused_result)) it will not generate warning anyway.

Best Regards, Vladislav Grishenko

-----Original Message-----
From: Petr Mensik <pemensik at redhat.com> 
Sent: Wednesday, July 10, 2019 3:01 PM
To: Vladislav Grishenko <themiron.ru at gmail.com>; dnsmasq-discuss at lists.thekelleys.org.uk
Subject: Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.

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-bounces at lists.thekelleys.org.uk> On Behalf Of Petr 
> Mensik
> Sent: Tuesday, July 9, 2019 5:31 PM
> To: dnsmasq-discuss at 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: pemensik at redhat.com  PGP: 65C6C973
> 

--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com  PGP: 65C6C973




More information about the Dnsmasq-discuss mailing list