5:21 p.m.
I sent V1 of this patch way back in October:
https://www.redhat.com/archives/libvir-list/2018-October/msg00889.html
but then self-NACKed it because I had written the patch to add the
network prefix to dhcp-range, but IPv4 requires the netmask rather
than range. Unfortunately, when I modified the patch accordingly the
getnameinfo() function began failing, I couldn't immediately see why,
and I had to pack everything up and go to KVM Forum. After that, I
promptly forgot to look back at it.
Today I finally revisited the problem (after the person who originally
found it sent a reminder), and discovered the failure of getnameinfo()
was due to a bug in a function that I added to the virSocketAddr
library in 2010!
Laine Stump (2):
util: set missing data length in virSocketAddrPrefixToNetmask()
network: add netmask to dhcp range of dnsmasq conf file for IPv4
src/network/bridge_driver.c | 27 +++++++++++++++----
src/util/virsocketaddr.c | 2 ++
.../dhcp6-nat-network.conf | 2 +-
.../networkxml2confdata/isolated-network.conf | 2 +-
.../nat-network-dns-srv-record-minimal.conf | 2 +-
.../nat-network-dns-srv-record.conf | 2 +-
.../nat-network-dns-txt-record.conf | 2 +-
.../networkxml2confdata/nat-network-mtu.conf | 2 +-
.../nat-network-name-with-quotes.conf | 2 +-
tests/networkxml2confdata/nat-network.conf | 2 +-
.../networkxml2confdata/netboot-network.conf | 2 +-
.../netboot-proxy-network.conf | 2 +-
.../networkxml2confdata/ptr-domains-auto.conf | 2 +-
13 files changed, 35 insertions(+), 16 deletions(-)
--
2.20.1
5:21 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: set missing data length in virSocketAddrPrefixToNetmask()
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask() with a virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
src/util/virsocketaddr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 4bc14bbd15..ccfaeabe13 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
netmask->data.inet4.sin_addr.s_addr = htonl(ip);
netmask->data.stor.ss_family = AF_INET;
+ netmask->len = sizeof(struct sockaddr_in);
result = 0;
} else if (family == AF_INET6) {
@@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
netmask->data.inet6.sin6_addr.s6_addr[i++] = 0;
}
netmask->data.stor.ss_family = AF_INET6;
+ netmask->len = sizeof(struct sockaddr_in6);
result = 0;
}
--
2.20.1
3:10 p.m.
New subject: [libvirt] [PATCHv2 1/2] util: set missing data length in virSocketAddrPrefixToNetmask()
On 2/18/19 6:21 PM, Laine Stump wrote:
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask() with a virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
src/util/virsocketaddr.c | 2 ++
1 file changed, 2 insertions(+)
I'm OK with this change as is - just curious on the below query...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 4bc14bbd15..ccfaeabe13 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
netmask->data.inet4.sin_addr.s_addr = htonl(ip);
netmask->data.stor.ss_family = AF_INET;
+ netmask->len = sizeof(struct sockaddr_in);
Hmmm.. how similar to virSocketAddrSetIPv4AddrNetOrder overall? I was
looking for "data\.inet.*=" via cscope and found that...
result = 0;
} else if (family == AF_INET6) {
@@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
netmask->data.inet6.sin6_addr.s6_addr[i++] = 0;
}
netmask->data.stor.ss_family = AF_INET6;
+ netmask->len = sizeof(struct sockaddr_in6);
My brain hurts thinking if similar to virSocketAddrSetIPv6AddrNetOrder
result = 0;
}
11:25 a.m.
New subject: [libvirt] [PATCHv2 1/2] util: set missing data length in virSocketAddrPrefixToNetmask()
On 2/20/19 4:10 PM, John Ferlan wrote:
On 2/18/19 6:21 PM, Laine Stump wrote:
> This fixes a bug that has been present since the original version of
> the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
> virSocketAddr::len was not being set.
>
> Apparently until now we were always calling
> virSocketAddrPrefixToNetmask() with a virSocketAddr object that was
> already (coincidentally) initialized for the proper address family,
> but the bug became apparent when trying to use it to fill in an
> otherwise uninitialized object.
>
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
> src/util/virsocketaddr.c | 2 ++
> 1 file changed, 2 insertions(+)
>
I'm OK with this change as is - just curious on the below query...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
> diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
> index 4bc14bbd15..ccfaeabe13 100644
> --- a/src/util/virsocketaddr.c
> +++ b/src/util/virsocketaddr.c
> @@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
> ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
> netmask->data.inet4.sin_addr.s_addr = htonl(ip);
> netmask->data.stor.ss_family = AF_INET;
> + netmask->len = sizeof(struct sockaddr_in);
Hmmm.. how similar to virSocketAddrSetIPv4AddrNetOrder overall? I was
looking for "data\.inet.*=" via cscope and found that...
Interesting. These 3 lines *are* identical in function to calling
virSocketAddrSetIPv4Addr() (the one that does an htonl() on the address).
> result = 0;
>
> } else if (family == AF_INET6) {
> @@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
> netmask->data.inet6.sin6_addr.s6_addr[i++] = 0;
> }
> netmask->data.stor.ss_family = AF_INET6;
> + netmask->len = sizeof(struct sockaddr_in6);
My brain hurts thinking if similar to virSocketAddrSetIPv6AddrNetOrder
Yeah. I'm not in the mood to figure out what's the right byte order for
the input to what happens in virSocketAddrSetIPv6Addr() and
virSocketAddrSetIPv6AddrNetOrder() is identical to what's currently done
in the IPv6 clause of virSocketAddrPrefixToNetmask(), so I think I'd
rather leave all of it for clarity's sake (and also to avoid the
potential of having my bug fix create yet another regression :-P)
5:21 p.m.
New subject: [libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible
security implication, which turned out to be unfounded) a bug on a
host where one of the interfaces was a superset of the libvirt network
where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
supplying the netmask for the /8 network to clients requesting an
address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
src/network/bridge_driver.c | 27 +++++++++++++++----
.../dhcp6-nat-network.conf | 2 +-
.../networkxml2confdata/isolated-network.conf | 2 +-
.../nat-network-dns-srv-record-minimal.conf | 2 +-
.../nat-network-dns-srv-record.conf | 2 +-
.../nat-network-dns-txt-record.conf | 2 +-
.../networkxml2confdata/nat-network-mtu.conf | 2 +-
.../nat-network-name-with-quotes.conf | 2 +-
tests/networkxml2confdata/nat-network.conf | 2 +-
.../networkxml2confdata/netboot-network.conf | 2 +-
.../netboot-proxy-network.conf | 2 +-
.../networkxml2confdata/ptr-domains-auto.conf | 2 +-
12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6d80818e40..9fa902896b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
!(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
- saddr, eaddr);
- if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
- virBufferAsprintf(&configbuf, ",%d", prefix);
- virBufferAddLit(&configbuf, "\n");
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
+ saddr, eaddr, prefix);
+ } else {
+ /* IPv4 - dnsmasq requires a netmask rather than prefix */
+ virSocketAddr netmask;
+ char *netmaskStr;
+
+ if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0)
{
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to translate bridge '%s'
"
+ "prefix %d to netmask"),
+ def->bridge, prefix);
+ goto cleanup;
+ }
+
+ if (!(netmaskStr = virSocketAddrFormat(&netmask)))
+ goto cleanup;
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
+ saddr, eaddr, netmaskStr);
+ VIR_FREE(netmaskStr);
+ }
VIR_FREE(saddr);
VIR_FREE(eaddr);
diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf
b/tests/networkxml2confdata/dhcp6-nat-network.conf
index d1058df3b6..536974e508 100644
--- a/tests/networkxml2confdata/dhcp6-nat-network.conf
+++ b/tests/networkxml2confdata/dhcp6-nat-network.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64
diff --git a/tests/networkxml2confdata/isolated-network.conf
b/tests/networkxml2confdata/isolated-network.conf
index ce4a59f6c1..693a83d9a0 100644
--- a/tests/networkxml2confdata/isolated-network.conf
+++ b/tests/networkxml2confdata/isolated-network.conf
@@ -10,7 +10,7 @@ bind-interfaces
listen-address=192.168.152.1
dhcp-option=3
no-resolv
-dhcp-range=192.168.152.2,192.168.152.254
+dhcp-range=192.168.152.2,192.168.152.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
index f35ea1d5d4..0b2ca6f5aa 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
@@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1
listen-address=fc00:db8:ac10:fd01::1
listen-address=10.24.10.1
srv-host=_name._tcp
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf
b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
index af1ed70758..a18c09aaa7 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
@@ -15,7 +15,7 @@ srv-host=_name4._tcp.test4.com,test4.example.com,4444
srv-host=_name5._udp,test5.example.com,1,55,555
srv-host=_name6._tcp.test6.com,test6.example.com,6666,0,666
srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.conf
b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
index 7f560fbb5c..735c261c01 100644
--- a/tests/networkxml2confdata/nat-network-dns-txt-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
@@ -9,7 +9,7 @@ except-interface=lo
bind-dynamic
interface=virbr0
txt-record=example,example value
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-mtu.conf
b/tests/networkxml2confdata/nat-network-mtu.conf
index 91b574b964..1dd4754f2a 100644
--- a/tests/networkxml2confdata/nat-network-mtu.conf
+++ b/tests/networkxml2confdata/nat-network-mtu.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.conf
b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
index 36e11d17b9..1b06de3066 100644
--- a/tests/networkxml2confdata/nat-network-name-with-quotes.conf
+++ b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
@@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1
listen-address=fc00:db8:ac10:fd01::1
listen-address=10.24.10.1
srv-host=_name._tcp
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network.conf
b/tests/networkxml2confdata/nat-network.conf
index a3c8b102d3..873a360acc 100644
--- a/tests/networkxml2confdata/nat-network.conf
+++ b/tests/networkxml2confdata/nat-network.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/netboot-network.conf
b/tests/networkxml2confdata/netboot-network.conf
index b554a5456c..99272b9d68 100644
--- a/tests/networkxml2confdata/netboot-network.conf
+++ b/tests/networkxml2confdata/netboot-network.conf
@@ -10,7 +10,7 @@ expand-hosts
except-interface=lo
bind-interfaces
listen-address=192.168.122.1
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
enable-tftp
diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf
b/tests/networkxml2confdata/netboot-proxy-network.conf
index afb4033f7e..fb0a20cff4 100644
--- a/tests/networkxml2confdata/netboot-proxy-network.conf
+++ b/tests/networkxml2confdata/netboot-proxy-network.conf
@@ -10,7 +10,7 @@ expand-hosts
except-interface=lo
bind-interfaces
listen-address=192.168.122.1
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-boot=pxeboot.img,,10.20.30.40
diff --git a/tests/networkxml2confdata/ptr-domains-auto.conf
b/tests/networkxml2confdata/ptr-domains-auto.conf
index 7f1a393dd5..86701c4ddf 100644
--- a/tests/networkxml2confdata/ptr-domains-auto.conf
+++ b/tests/networkxml2confdata/ptr-domains-auto.conf
@@ -10,7 +10,7 @@ local=/1.0.e.f.0.1.c.a.8.b.d.0.1.0.0.2.ip6.arpa/
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
--
2.20.1
3:10 p.m.
New subject: [libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
On 2/18/19 6:21 PM, Laine Stump wrote:
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible
security implication, which turned out to be unfounded) a bug on a
host where one of the interfaces was a superset of the libvirt network
where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
supplying the netmask for the /8 network to clients requesting an
address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
src/network/bridge_driver.c | 27 +++++++++++++++----
.../dhcp6-nat-network.conf | 2 +-
.../networkxml2confdata/isolated-network.conf | 2 +-
.../nat-network-dns-srv-record-minimal.conf | 2 +-
.../nat-network-dns-srv-record.conf | 2 +-
.../nat-network-dns-txt-record.conf | 2 +-
.../networkxml2confdata/nat-network-mtu.conf | 2 +-
.../nat-network-name-with-quotes.conf | 2 +-
tests/networkxml2confdata/nat-network.conf | 2 +-
.../networkxml2confdata/netboot-network.conf | 2 +-
.../netboot-proxy-network.conf | 2 +-
.../networkxml2confdata/ptr-domains-auto.conf | 2 +-
12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6d80818e40..9fa902896b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
!(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
- saddr, eaddr);
- if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
- virBufferAsprintf(&configbuf, ",%d", prefix);
- virBufferAddLit(&configbuf, "\n");
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
+ saddr, eaddr, prefix);
+ } else {
+ /* IPv4 - dnsmasq requires a netmask rather than prefix */
+ virSocketAddr netmask;
Does it matter if this isn't initialized? e.g. "= { 0 };"
+ char *netmaskStr;
VIR_AUTOFREE(char *) netmaskStr = NULL;
+
+ if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0)
{
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to translate bridge '%s'
"
+ "prefix %d to netmask"),
+ def->bridge, prefix);
+ goto cleanup;
+ }
+
+ if (!(netmaskStr = virSocketAddrFormat(&netmask)))
+ goto cleanup;
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
+ saddr, eaddr, netmaskStr);
+ VIR_FREE(netmaskStr);
w/ VIR_AUTOFREE, this isn't necessary
I know bridge_driver doesn't use it yet, but doesn't harm to start.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
11:52 a.m.
New subject: [libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
On 2/20/19 4:10 PM, John Ferlan wrote:
On 2/18/19 6:21 PM, Laine Stump wrote:
> dnsmasq documentation says that the *IPv4* prefix/network
> address/broadcast address sent to dhcp clients will be automatically
> determined by dnsmasq by looking at the interface it's listening on,
> so the original libvirt code did not add a netmask to the dnsmasq
> commandline (or later, the dnsmasq conf file).
>
> For *IPv6* however, dnsmasq apparently cannot automatically determine
> the prefix (functionally the same as a netmask), and it must be
> explicitly provided in the conf file (as a part of the dhcp-range
> option). So many years after IPv4 DHCP support had been added, when
> IPv6 dhcp support was added the prefix was included at the end of the
> dhcp-range setting, but only for IPv6.
>
> Recently a user reported (privately, because they suspected a possible
> security implication, which turned out to be unfounded) a bug on a
> host where one of the interfaces was a superset of the libvirt network
> where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
> the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
> supplying the netmask for the /8 network to clients requesting an
> address on the /24 interface.
>
> This seems like a bug in dnsmasq, but even if/when it gets fixed
> there, it looks like there is no harm in just always adding the
> netmask to all IPv4 dhcp-range options similar to how prefix is added
> to all IPv6 dhcp-range options.
>
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
> src/network/bridge_driver.c | 27 +++++++++++++++----
> .../dhcp6-nat-network.conf | 2 +-
> .../networkxml2confdata/isolated-network.conf | 2 +-
> .../nat-network-dns-srv-record-minimal.conf | 2 +-
> .../nat-network-dns-srv-record.conf | 2 +-
> .../nat-network-dns-txt-record.conf | 2 +-
> .../networkxml2confdata/nat-network-mtu.conf | 2 +-
> .../nat-network-name-with-quotes.conf | 2 +-
> tests/networkxml2confdata/nat-network.conf | 2 +-
> .../networkxml2confdata/netboot-network.conf | 2 +-
> .../netboot-proxy-network.conf | 2 +-
> .../networkxml2confdata/ptr-domains-auto.conf | 2 +-
> 12 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6d80818e40..9fa902896b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
> !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
> goto cleanup;
>
> - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
> - saddr, eaddr);
> - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> - virBufferAsprintf(&configbuf, ",%d", prefix);
> - virBufferAddLit(&configbuf, "\n");
> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
> + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
> + saddr, eaddr, prefix);
> + } else {
> + /* IPv4 - dnsmasq requires a netmask rather than prefix */
> + virSocketAddr netmask;
Does it matter if this isn't initialized? e.g. "= { 0 };"
None of the other instances of virSocketAddr are initialized to 0, and
the intent is that the utility functions should fill in all fields that
are relevant. I'd rather not be the one responsible for starting a cargo
cult of explicitly initializing them when it's not necessary :-)
> + char *netmaskStr;
VIR_AUTOFREE(char *) netmaskStr = NULL;
> +
> + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) <
0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to translate bridge '%s'
"
> + "prefix %d to netmask"),
> + def->bridge, prefix);
> + goto cleanup;
> + }
> +
> + if (!(netmaskStr = virSocketAddrFormat(&netmask)))
> + goto cleanup;
> + virBufferAsprintf(&configbuf,
"dhcp-range=%s,%s,%s\n",
> + saddr, eaddr, netmaskStr);
> + VIR_FREE(netmaskStr);
w/ VIR_AUTOFREE, this isn't necessary
I know bridge_driver doesn't use it yet, but doesn't harm to start.
Yeah, I'll do that before I push.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
2102
days inactive
2105
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Laine Stump