Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.
Hi,
because there is more patches, I created a pull request in my fork on
github [1]. I think it is somehow more handy than attachments here. I
think all of them are already sent. Attaching complete set here again in
one place. Also made Fedora bug #1728701 [2].
We need a fix because we are hitting this issue in OpenStack a lot. I
would like to include fix and would like it to be the same way on
upstream. If there is different way to fix it, please say so.
Any more opinions on patches 4 and 5?
1. https://github.com/InfrastructureServices/dnsmasq/pull/1
2. https://bugzilla.redhat.com/show_bug.cgi?id=1728701
On 7/10/19 1:34 PM, Vladislav Grishenko wrote:
> 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 sockets are
> created, on subsequent interface up with the same address - bind error raised
> and only UDP socket is created additionally (unlike before).
Fixed now.
>
> 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.
I changed only things related to changes I did. We sometime run Coverity
scans on the codebase, and such issues arise there. So I make it the way
no new issues are discovered by my changes.
On the other hand, maybe it should print complete address with port as
well. Precise information does not hurt, even when port would be a bit
repeating. Now it includes also port. These are just debug information
after all, no need for them to be shortest possible.
>
> Best Regards, Vladislav Grishenko
>
> -Original Message-
> From: Petr Mensik
> Sent: Wednesday, July 10, 2019 3:01 PM
> To: Vladislav Grishenko ;
> dnsmasq-discuss@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 fir
Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.
Hi Vladislav,
I think I found usable solution for duplicate addresses. I think there
is many list of interfaces without clear distinction between them. For
example, daemon->int_names contains all addresses quite similar to
daemon->interfaces. And there is also if_names and if_addrs. I were able
just decipher interfaces is always made from real interface names found
on system. No wildcards are used.
Anyway, made two new patches on top of previous. First includes address
in listener structure and check existing addresses before creating new
address. If there is already such listener, note it is used more times.
If one interface stops listening, do not stop listening yet until all
listeners end.
Because address includes family, patch 5 removes duplicate family. Made
that separate because it kind of cover what is important change.
Cheers,
Petr
On 7/11/19 12:23 PM, Petr Mensik wrote:
> Oh, tested it and confirm it breaks the same IP addreses listeners. If
> the same address is used, second address listener is not created
> successfully. It reports:
>
> dnsmasq: failed to create listening socket for 10.129.1.1: Address
> already in use
>
> When the first one is destroyed, second one cannot take over it.
> Unfortunately, it seems interfaces list works as unique addresses at the
> same time.
>
> One possible solution is socket option SO_REUSEPORT, which would allow
> binding multiple times to the same address. However, it seems its
> support is not very portable, at least on stack overflow article. It
> might create more problem than it solves. I think its usage should be
> configurable. Not a good solution.
>
> There is also secondary list in daemon->if_addrs, which maybe should be
> used instead to create real listeners. Now it creates listeners for
> explicitly given addresses without interface name.
>
> Unless TCP queries should stop checking interface indexes, list of
> current indexes is required somewhere. I think daemon->interfaces is the
> best list for it. But when creating listeners, just single should be
> created for a shared address. Current structures do not allow such
> sharing AFAIK. Either irec should be only one for single physical
> interface and should have address records for each address nested.
>
> I am looking on possible solutions. I do not have any good solution yet.
> I doubt it can be solved with as little changes as I used before.
>
> 1.
> https://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-so-reuseport-how-do-they-differ-do-they-mean-t
>
> On 7/10/19 1:34 PM, Vladislav Grishenko wrote:
>> 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 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
>>
&g
Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.
Oh, tested it and confirm it breaks the same IP addreses listeners. If
the same address is used, second address listener is not created
successfully. It reports:
dnsmasq: failed to create listening socket for 10.129.1.1: Address
already in use
When the first one is destroyed, second one cannot take over it.
Unfortunately, it seems interfaces list works as unique addresses at the
same time.
One possible solution is socket option SO_REUSEPORT, which would allow
binding multiple times to the same address. However, it seems its
support is not very portable, at least on stack overflow article. It
might create more problem than it solves. I think its usage should be
configurable. Not a good solution.
There is also secondary list in daemon->if_addrs, which maybe should be
used instead to create real listeners. Now it creates listeners for
explicitly given addresses without interface name.
Unless TCP queries should stop checking interface indexes, list of
current indexes is required somewhere. I think daemon->interfaces is the
best list for it. But when creating listeners, just single should be
created for a shared address. Current structures do not allow such
sharing AFAIK. Either irec should be only one for single physical
interface and should have address records for each address nested.
I am looking on possible solutions. I do not have any good solution yet.
I doubt it can be solved with as little changes as I used before.
1.
https://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-so-reuseport-how-do-they-differ-do-they-mean-t
On 7/10/19 1:34 PM, Vladislav Grishenko wrote:
> 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 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
> Sent: Wednesday, July 10, 2019 3:01 PM
> To: Vladislav Grishenko ;
> dnsmasq-discuss@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 remove
Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.
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 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
Sent: Wednesday, July 10, 2019 3:01 PM
To: Vladislav Grishenko ;
dnsmasq-discuss@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
> 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 t
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 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
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, , netmask, prefix, 0);
}
-
+
+/*
+ * Clean old interfaces no longer found.
+ */
+static void clean_interfaces()
+{
+ struct irec *iface;
+ struct irec **up = >interfaces;
+
+ for (iface = *up; iface; iface = *up)
+ {
+if (!iface->found && !iface->done)
+ {
+*up = iface->next;
+free(iface->name);
+free(iface);
+ }
+else
+ {
+up = >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.
Re: [Dnsmasq-discuss] [PATCH] Issues with TCP queries on recreated interfaces.
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) 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 * function args on the next line are not aligned with the first argument * prettyprint_addr() result is forcibly ignored with (void) unlike other places Best Regards, Vladislav Grishenko -Original Message- From: Dnsmasq-discuss 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 ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
