[libvirt] [PATCH v3 0/4] don't masquerade local broadcast/multicast packets
Masquerading local broadcast breaks DHCP replies for some clients. There has been a report about broken local multicast too. (See references in the patches.) Regarding multicast, right now the series disables masquerading for the most restrictive local multicast range only. v2->v3 changes: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine]. - Pass (address, prefix) pairs as both source and destination parameters to these functions. - Introduce virPfxSocketAddr structure for simpler handling of said (address, prefix) pairs. - Also prevent masquerading of directed broadcast [Laine]. - Start to get serious about pointers-to-const. Testing: - "make check" and "make syntax-check" pass, - thanks to the great docs on libvirt.org (compiling & deployment) I even managed to test this on my RHEL-6 laptop, with repeated net-start / net-destroy commands. Chain POSTROUTING (policy ACCEPT 0 packets, 0 bytes) pkts bytes target prot opt in out source destination 0 0 RETURN all -- * * 192.168.122.0/24 224.0.0.0/24 0 0 RETURN all -- * * 192.168.122.0/24 192.168.122.255 0 0 RETURN all -- * * 192.168.122.0/24 255.255.255.255 0 0 MASQUERADE tcp -- * * 192.168.122.0/24 !192.168.122.0/24 masq ports: 1024-65535 0 0 MASQUERADE udp -- * * 192.168.122.0/24 !192.168.122.0/24 masq ports: 1024-65535 0 0 MASQUERADE all -- * * 192.168.122.0/24 !192.168.122.0/24 Laszlo Ersek (4): iptablesFormatNetwork(): constify target of "netaddr" parameter util/viriptables: add/remove rules that short-circuit masquerading virSocketAddrBroadcastByPrefix(): constify target of "addr" parameter bridge driver: don't masquerade local subnet broadcast/multicast packets src/util/viriptables.h | 11 +++ src/util/virsocketaddr.h | 8 +- src/network/bridge_driver_linux.c | 151 +++++++++++++++++++++++++++++++++++++- src/util/viriptables.c | 84 ++++++++++++++++++++- src/util/virsocketaddr.c | 8 +- src/libvirt_private.syms | 2 + 6 files changed, 251 insertions(+), 13 deletions(-) -- 1.8.3.1
... and adapt functions that would cast away the new const qualifier. Given typedef virSocketAddr *virSocketAddrPtr; Compare the parse trees of the following two declarations: (a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr; (a) parses as const virSocketAddrPtr addr | | | | | identifier | | | | typedef-name direct-declarator | | | | type-specifier declarator | | | type-qualifier declaration-specifiers init-declarator \ / | declaration-specifiers init-declarator-list \ / declaration The meaning of this declaration is "constant object called 'addr' with type 'virSocketAddrPtr'. That is, "const" does not penetrate "virSocketAddrPtr". It means /* constant pointer to variable object */ virSocketAddr *const addr; (b) parses as const virSocketAddr * addr | | | | | | | identifier | | | | | typedef-name pointer direct-declarator | | \ / | type-specifier declarator | | | type-qualifier declaration-specifiers init-declarator \ / | declaration-specifiers init-declarator-list \ / declaration It means /* variable pointer to constant object */ const virSocketAddr *addr; For now only functions "rooted" in iptablesFormatNetwork() are converted. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- src/util/virsocketaddr.h | 4 ++-- src/util/viriptables.c | 2 +- src/util/virsocketaddr.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index b28fe6c..14b3a8b 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -99,10 +99,10 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask); int virSocketAddrCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, virSocketAddrPtr netmask); -int virSocketAddrMask(const virSocketAddrPtr addr, +int virSocketAddrMask(const virSocketAddr *addr, const virSocketAddrPtr netmask, virSocketAddrPtr network); -int virSocketAddrMaskByPrefix(const virSocketAddrPtr addr, +int virSocketAddrMaskByPrefix(const virSocketAddr *addr, unsigned int prefix, virSocketAddrPtr network); int virSocketAddrBroadcast(const virSocketAddrPtr addr, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 0f57d66..52e2bde 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -241,7 +241,7 @@ iptablesRemoveUdpInput(int family, } -static char *iptablesFormatNetwork(virSocketAddr *netaddr, +static char *iptablesFormatNetwork(const virSocketAddr *netaddr, unsigned int prefix) { virSocketAddr network; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 3e01baf..0d415df 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -399,7 +399,7 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) { * Returns 0 in case of success, or -1 on error. */ int -virSocketAddrMask(const virSocketAddrPtr addr, +virSocketAddrMask(const virSocketAddr *addr, const virSocketAddrPtr netmask, virSocketAddrPtr network) { @@ -445,7 +445,7 @@ virSocketAddrMask(const virSocketAddrPtr addr, * Returns 0 in case of success, or -1 on error. */ int -virSocketAddrMaskByPrefix(const virSocketAddrPtr addr, +virSocketAddrMaskByPrefix(const virSocketAddr *addr, unsigned int prefix, virSocketAddrPtr network) { -- 1.8.3.1
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
... and adapt functions that would cast away the new const qualifier.
Given
typedef virSocketAddr *virSocketAddrPtr;
Compare the parse trees of the following two declarations:
(a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr;
Umm.. Eric? A little help? :-)
Il 24/09/2013 10:46, Laine Stump ha scritto:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
... and adapt functions that would cast away the new const qualifier.
Given
typedef virSocketAddr *virSocketAddrPtr;
Compare the parse trees of the following two declarations:
(a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr;
Umm.. Eric? A little help? :-)
Heh :) In more layman terms, you can read types out in English like this: (1) read the type from right to the left (2) if "const" is the first token, read it as "that is constant" and associate it to the closest type. Alternatively, move such a "const" right but no further than the first *. (3) if "const" is anywhere else, read it as "constant" So: const virSocketAddrPtr addr ("addr is a virSocketAddrPtr that is constant") -> virSocketAddrPtr const addr ("addr is a constant virSocketAddrPtr") -> virSocketAddr * const addr ("addr is a constant pointer to virSocketAddr const virSocketAddr *addr ("addr is a pointer to a virSocketAddr that is constant") -> virSocketAddr const *addr ("addr is a pointer to a constant virSocketAddr") Laszlo, correct me if I'm wrong. I suspect that forbidding const.*Ptr in "make syntax-check" wouldn't be a bad idea. Paolo
On 09/24/13 15:01, Paolo Bonzini wrote:
Il 24/09/2013 10:46, Laine Stump ha scritto:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
... and adapt functions that would cast away the new const qualifier.
Given
typedef virSocketAddr *virSocketAddrPtr;
Compare the parse trees of the following two declarations:
(a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr;
Umm.. Eric? A little help? :-)
Heh :)
In more layman terms, you can read types out in English like this:
(1) read the type from right to the left
(2) if "const" is the first token, read it as "that is constant" and associate it to the closest type. Alternatively, move such a "const" right but no further than the first *.
(3) if "const" is anywhere else, read it as "constant"
So:
const virSocketAddrPtr addr ("addr is a virSocketAddrPtr that is constant") -> virSocketAddrPtr const addr ("addr is a constant virSocketAddrPtr") -> virSocketAddr * const addr ("addr is a constant pointer to virSocketAddr
const virSocketAddr *addr ("addr is a pointer to a virSocketAddr that is constant") -> virSocketAddr const *addr ("addr is a pointer to a constant virSocketAddr")
Laszlo, correct me if I'm wrong.
Never thought of it this way before, but it does seem to work in the examples above :)
I suspect that forbidding const.*Ptr in "make syntax-check" wouldn't be a bad idea.
The pattern should probably require some whitespace in the middle as well, if constWhateverPtr typedefs are to be accepted as valid. Thanks Laszlo
On 09/24/2013 07:33 AM, Laszlo Ersek wrote:
I suspect that forbidding const.*Ptr in "make syntax-check" wouldn't be a bad idea.
The pattern should probably require some whitespace in the middle as well, if constWhateverPtr typedefs are to be accepted as valid.
Here's the proposed syntax check: diff --git i/cfg.mk w/cfg.mk index dad8a90..6a17d43 100644 --- i/cfg.mk +++ w/cfg.mk @@ -468,6 +468,14 @@ sc_correct_id_types: halt="use pid_t for pid, uid_t for uid, gid_t for gid" \ $(_sc_search_regexp) +# 'const fooPtr a' is the same as 'foo * const a', even though it is +# usually desired to have 'foo const *a'. It's easier to just prevent +# the confusing mix of typedef vs. const placement. +sc_forbid_const_pointer_typedef: + @prohibit='const [a-zA-Z_0-9]*Ptr' \ + halt="'const fooPtr var' does not declare what you meant" \ + $(_sc_search_regexp) + # Forbid sizeof foo or sizeof (foo), require sizeof(foo) sc_size_of_brackets: @prohibit='sizeof\s' \ and here's the damage we'd have to clean up: $ make sc_forbid_const_pointer_typedef | wc maint.mk: 'const fooPtr var' does not declare what you meant make: *** [sc_forbid_const_pointer_typedef] Error 1 403 1766 37136 spread among 75 files. There's probably some fallout, too - once you have a const-correct pointer type, it might show us places where we have been assigning through what we thought was const. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/24/13 18:25, Eric Blake wrote:
On 09/24/2013 07:33 AM, Laszlo Ersek wrote:
I suspect that forbidding const.*Ptr in "make syntax-check" wouldn't be a bad idea.
The pattern should probably require some whitespace in the middle as well, if constWhateverPtr typedefs are to be accepted as valid.
Here's the proposed syntax check:
diff --git i/cfg.mk w/cfg.mk index dad8a90..6a17d43 100644 --- i/cfg.mk +++ w/cfg.mk @@ -468,6 +468,14 @@ sc_correct_id_types: halt="use pid_t for pid, uid_t for uid, gid_t for gid" \ $(_sc_search_regexp)
+# 'const fooPtr a' is the same as 'foo * const a', even though it is +# usually desired to have 'foo const *a'. It's easier to just prevent +# the confusing mix of typedef vs. const placement. +sc_forbid_const_pointer_typedef: + @prohibit='const [a-zA-Z_0-9]*Ptr' \ + halt="'const fooPtr var' does not declare what you meant" \ + $(_sc_search_regexp) + # Forbid sizeof foo or sizeof (foo), require sizeof(foo) sc_size_of_brackets: @prohibit='sizeof\s' \
and here's the damage we'd have to clean up:
$ make sc_forbid_const_pointer_typedef | wc maint.mk: 'const fooPtr var' does not declare what you meant make: *** [sc_forbid_const_pointer_typedef] Error 1 403 1766 37136
spread among 75 files.
There's probably some fallout, too - once you have a const-correct pointer type, it might show us places where we have been assigning through what we thought was const.
Likely not actively assigning (which would be undefined behavior of coure), just perhaps casting away real constness (which itself is not undefined behavior). But gcc would "help" with those. I'm not a convincing stakeholder in libvirt (yet! :) ), so I'm not suggesting to go ahead with it. But the pattern looks good (assuming you have an LC_ALL=C setting somewhere in the checker, since regex ranges depend on collation order, but I'm 100% sure I don't have to say that, and the setting is already there.) Thanks, laszlo
On 09/24/13 10:46, Laine Stump wrote:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
... and adapt functions that would cast away the new const qualifier.
Given
typedef virSocketAddr *virSocketAddrPtr;
Compare the parse trees of the following two declarations:
(a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr;
Umm.. Eric? A little help? :-)
The grammar rules that I used for the AST derivation can be looked up eg. in the final C11 draft, http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf Section 6.7 "Declarations". But, the short version is really just that type qualifiers (like const & volatile) don't enter the typedef name; they qualify the variable being declared. const virSocketAddrPtr addr; virSocketAddrPtr const addr; these are equivalent, they mean the same thing, a constant pointer to a variable object. Expanding the typedef, that's written as virSocketAddr *const addr; (It is closer in appearance to the second form above.) If you want to qualify the target of the pointer, you must say one of the following: (i) const virSocketAddr *addr; (ii) virSocketAddr const *addr; (iii) typedef const virSocketAddr *constVirSocketAddrPtr; constVirSocketAddrPtr addr; (iv) typedef virSocketAddr const *constVirSocketAddrPtr; constVirSocketAddrPtr addr; In general I disapprove of typedefs: they seem to be friendly by saving you the repeated typing of "struct" and "*". Until they trick you :) Laszlo
On 09/24/2013 09:03 AM, Laszlo Ersek wrote:
On 09/24/13 10:46, Laine Stump wrote:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
... and adapt functions that would cast away the new const qualifier.
Given
typedef virSocketAddr *virSocketAddrPtr;
Compare the parse trees of the following two declarations:
(a) const virSocketAddrPtr addr; (b) const virSocketAddr *addr; Umm.. Eric? A little help? :-) The grammar rules that I used for the AST derivation can be looked up eg. in the final C11 draft,
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
Section 6.7 "Declarations".
But, the short version is really just that type qualifiers (like const & volatile) don't enter the typedef name; they qualify the variable being declared.
I like your explanation better :-)
In general I disapprove of typedefs: they seem to be friendly by saving you the repeated typing of "struct" and "*". Until they trick you :)
I like typedefs for eliminating repetitive typing of "struct", but not for removing "*" - that seems pointless to me (pun not intended), since you're not saving any characters, and losing track of the nice "*" that everyone is used to seeing.
The functions - iptablesAddDontMasquerade(), - iptablesRemoveDontMasquerade() handle exceptions in the masquerading implemented in the POSTROUTING chain of the "nat" table. Such exceptions should be added as chronologically latest, logically top-most rules. The bridge driver will call these functions beginning with the next patch: some special destination IP addresses always refer to the local subnetwork, even though they don't match any practical subnetwork's netmask. Packets from virbrN targeting such IP addresses are never routed outwards, but the current rules treat them as non-virbrN-destined packets and masquerade them. This causes problems for some receivers on virbrN. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- v2->v3: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine]. - Pass (address, prefix) pairs as both source and destination parameters to these functions. - Introduce virPfxSocketAddr structure for simpler handling of said (address, prefix) pairs. src/util/viriptables.h | 11 +++++++ src/util/viriptables.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 95 insertions(+) diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 447f4a8..fcff5f8 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -26,6 +26,11 @@ # include "virsocketaddr.h" +typedef struct { + virSocketAddr addr; + unsigned int prefix; +} virPfxSocketAddr; + int iptablesAddTcpInput (int family, const char *iface, int port); @@ -94,6 +99,12 @@ int iptablesRemoveForwardMasquerade (virSocketAddr *netaddr, virSocketAddrRangePtr addr, virPortRangePtr port, const char *protocol); +int iptablesAddDontMasquerade (const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst); +int iptablesRemoveDontMasquerade (const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst); int iptablesAddOutputFixUdpChecksum (const char *iface, int port); int iptablesRemoveOutputFixUdpChecksum (const char *iface, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 52e2bde..90fe900 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -832,6 +832,88 @@ iptablesRemoveForwardMasquerade(virSocketAddr *netaddr, } +/* Don't masquerade traffic coming from the network associated with the bridge + * if said traffic targets @destaddr/@destprefix. + */ +static int +iptablesDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst, + int action) +{ + int ret = -1; + char *srcStr = NULL; + char *dstStr = NULL; + virCommandPtr cmd = NULL; + + if (!(srcStr = iptablesFormatNetwork(&src->addr, src->prefix))) + return -1; + + if (!(dstStr = iptablesFormatNetwork(&dst->addr, dst->prefix))) + goto cleanup; + + if (!VIR_SOCKET_ADDR_IS_FAMILY(&src->addr, AF_INET)) { + /* Higher level code *should* guaranteee it's impossible to get here. */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempted to NAT '%s'. NAT is only supported for IPv4."), + srcStr); + goto cleanup; + } + + cmd = iptablesCommandNew("nat", "POSTROUTING", AF_INET, action); + + if (physdev && physdev[0]) + virCommandAddArgList(cmd, "--out-interface", physdev, NULL); + + virCommandAddArgList(cmd, "--source", srcStr, "--destination", dstStr, + "--jump", "RETURN", NULL); + ret = virCommandRun(cmd, NULL); +cleanup: + virCommandFree(cmd); + VIR_FREE(dstStr); + VIR_FREE(srcStr); + return ret; +} + +/** + * iptablesAddDontMasquerade: + * @src: the source network address and prefix + * @physdev: the physical output device or NULL + * @destaddr: the destination network address and prefix + * + * Add rules to the "nat" IP table to avoid masquerading from @src/srcprefix to + * @dst/dstprefix on @physdev. + * + * Returns 0 in case of success and -1 on failure. + */ +int +iptablesAddDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst) +{ + return iptablesDontMasquerade(src, physdev, dst, ADD); +} + +/** + * iptablesRemoveDontMasquerade: + * @src: the source network address and prefix + * @physdev: the physical output device or NULL + * @destaddr: the destination network address and prefix + * + * Remove rules from the "nat" IP table that prevent masquerading from + * @src/srcprefix to @dst/dstprefix on @physdev. + * + * Returns 0 in case of success and -1 on failure. + */ +int +iptablesRemoveDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst) +{ + return iptablesDontMasquerade(src, physdev, dst, REMOVE); +} + + static int iptablesOutputFixUdpChecksum(const char *iface, int port, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 50e2f48..fb212ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1446,6 +1446,7 @@ virInitctlSetRunLevel; # util/viriptables.h +iptablesAddDontMasquerade; iptablesAddForwardAllowCross; iptablesAddForwardAllowIn; iptablesAddForwardAllowOut; @@ -1456,6 +1457,7 @@ iptablesAddForwardRejectOut; iptablesAddOutputFixUdpChecksum; iptablesAddTcpInput; iptablesAddUdpInput; +iptablesRemoveDontMasquerade; iptablesRemoveForwardAllowCross; iptablesRemoveForwardAllowIn; iptablesRemoveForwardAllowOut; -- 1.8.3.1
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
The functions - iptablesAddDontMasquerade(), - iptablesRemoveDontMasquerade() handle exceptions in the masquerading implemented in the POSTROUTING chain of the "nat" table. Such exceptions should be added as chronologically latest, logically top-most rules.
The bridge driver will call these functions beginning with the next patch: some special destination IP addresses always refer to the local subnetwork, even though they don't match any practical subnetwork's netmask. Packets from virbrN targeting such IP addresses are never routed outwards, but the current rules treat them as non-virbrN-destined packets and masquerade them. This causes problems for some receivers on virbrN.
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- v2->v3: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine]. - Pass (address, prefix) pairs as both source and destination parameters to these functions. - Introduce virPfxSocketAddr structure for simpler handling of said (address, prefix) pairs.
src/util/viriptables.h | 11 +++++++ src/util/viriptables.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 95 insertions(+)
diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 447f4a8..fcff5f8 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -26,6 +26,11 @@
# include "virsocketaddr.h"
+typedef struct { + virSocketAddr addr; + unsigned int prefix; +} virPfxSocketAddr; +
I'm conflicted about whether or not it's really beneficial to have this struct (is the setup really worth it just to save one arg?), but I suppose it doesn't hurt anything, so I'll go along with it if nobody else objects. It *does* make these functions' APIs inconsistent with the other iptables functions that take a separate addr and prefix, though - if we keep this then we should probably update the rest of the API to be consistent (not in this patch though).
int iptablesAddTcpInput (int family, const char *iface, int port); @@ -94,6 +99,12 @@ int iptablesRemoveForwardMasquerade (virSocketAddr *netaddr, virSocketAddrRangePtr addr, virPortRangePtr port, const char *protocol); +int iptablesAddDontMasquerade (const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst); +int iptablesRemoveDontMasquerade (const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst); int iptablesAddOutputFixUdpChecksum (const char *iface, int port); int iptablesRemoveOutputFixUdpChecksum (const char *iface, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 52e2bde..90fe900 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -832,6 +832,88 @@ iptablesRemoveForwardMasquerade(virSocketAddr *netaddr, }
+/* Don't masquerade traffic coming from the network associated with the bridge + * if said traffic targets @destaddr/@destprefix. + */ +static int +iptablesDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst, + int action) +{ + int ret = -1; + char *srcStr = NULL; + char *dstStr = NULL; + virCommandPtr cmd = NULL; + + if (!(srcStr = iptablesFormatNetwork(&src->addr, src->prefix))) + return -1; + + if (!(dstStr = iptablesFormatNetwork(&dst->addr, dst->prefix))) + goto cleanup; + + if (!VIR_SOCKET_ADDR_IS_FAMILY(&src->addr, AF_INET)) { + /* Higher level code *should* guaranteee it's impossible to get here. */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempted to NAT '%s'. NAT is only supported for IPv4."), + srcStr); + goto cleanup; + }
Don't you need to do the same check for dst?
+ + cmd = iptablesCommandNew("nat", "POSTROUTING", AF_INET, action); + + if (physdev && physdev[0]) + virCommandAddArgList(cmd, "--out-interface", physdev, NULL); + + virCommandAddArgList(cmd, "--source", srcStr, "--destination", dstStr, + "--jump", "RETURN", NULL); + ret = virCommandRun(cmd, NULL); +cleanup: + virCommandFree(cmd); + VIR_FREE(dstStr); + VIR_FREE(srcStr); + return ret; +} + +/** + * iptablesAddDontMasquerade: + * @src: the source network address and prefix + * @physdev: the physical output device or NULL + * @destaddr: the destination network address and prefix + * + * Add rules to the "nat" IP table to avoid masquerading from @src/srcprefix to + * @dst/dstprefix on @physdev. + * + * Returns 0 in case of success and -1 on failure. + */ +int +iptablesAddDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst) +{ + return iptablesDontMasquerade(src, physdev, dst, ADD); +} + +/** + * iptablesRemoveDontMasquerade: + * @src: the source network address and prefix + * @physdev: the physical output device or NULL + * @destaddr: the destination network address and prefix + * + * Remove rules from the "nat" IP table that prevent masquerading from + * @src/srcprefix to @dst/dstprefix on @physdev. + * + * Returns 0 in case of success and -1 on failure. + */ +int +iptablesRemoveDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst) +{ + return iptablesDontMasquerade(src, physdev, dst, REMOVE); +} + + static int iptablesOutputFixUdpChecksum(const char *iface, int port, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 50e2f48..fb212ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1446,6 +1446,7 @@ virInitctlSetRunLevel;
# util/viriptables.h +iptablesAddDontMasquerade; iptablesAddForwardAllowCross; iptablesAddForwardAllowIn; iptablesAddForwardAllowOut; @@ -1456,6 +1457,7 @@ iptablesAddForwardRejectOut; iptablesAddOutputFixUdpChecksum; iptablesAddTcpInput; iptablesAddUdpInput; +iptablesRemoveDontMasquerade; iptablesRemoveForwardAllowCross; iptablesRemoveForwardAllowIn; iptablesRemoveForwardAllowOut;
On 09/24/13 11:24, Laine Stump wrote:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 447f4a8..fcff5f8 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -26,6 +26,11 @@
# include "virsocketaddr.h"
+typedef struct { + virSocketAddr addr; + unsigned int prefix; +} virPfxSocketAddr; +
I'm conflicted about whether or not it's really beneficial to have this struct (is the setup really worth it just to save one arg?),
It is one arg per address (ie. one for src, one for dst), and this is multiplied by the number of function calls that pass them.
but I suppose it doesn't hurt anything, so I'll go along with it if nobody else objects. It *does* make these functions' APIs inconsistent with the other iptables functions that take a separate addr and prefix, though - if we keep this then we should probably update the rest of the API to be consistent (not in this patch though).
I don't insist of course -- it seemed to save a lot of boilerplate for me, but avoiding inconsistency (or extra churn) can easily trump that.
+/* Don't masquerade traffic coming from the network associated with the bridge + * if said traffic targets @destaddr/@destprefix. + */ +static int +iptablesDontMasquerade(const virPfxSocketAddr *src, + const char *physdev, + const virPfxSocketAddr *dst, + int action) +{ + int ret = -1; + char *srcStr = NULL; + char *dstStr = NULL; + virCommandPtr cmd = NULL; + + if (!(srcStr = iptablesFormatNetwork(&src->addr, src->prefix))) + return -1; + + if (!(dstStr = iptablesFormatNetwork(&dst->addr, dst->prefix))) + goto cleanup; + + if (!VIR_SOCKET_ADDR_IS_FAMILY(&src->addr, AF_INET)) { + /* Higher level code *should* guaranteee it's impossible to get here. */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attempted to NAT '%s'. NAT is only supported for IPv4."), + srcStr); + goto cleanup; + }
Don't you need to do the same check for dst?
Hm, yeah I do. But, since we don't need the "machinery" for 192.168.122.255/32: how about I go back to v2, do the renames that you asked for, and submit a v4 just like that? No fiddling with pointer typedefs, no runtime computation of addresses, and so on. Stick to the bare minimum? Thanks! Laszlo
... and adapt functions that would cast away the new const qualifier. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- src/util/virsocketaddr.h | 4 ++-- src/util/virsocketaddr.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 14b3a8b..14113e0 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -105,10 +105,10 @@ int virSocketAddrMask(const virSocketAddr *addr, int virSocketAddrMaskByPrefix(const virSocketAddr *addr, unsigned int prefix, virSocketAddrPtr network); -int virSocketAddrBroadcast(const virSocketAddrPtr addr, +int virSocketAddrBroadcast(const virSocketAddr *addr, const virSocketAddrPtr netmask, virSocketAddrPtr broadcast); -int virSocketAddrBroadcastByPrefix(const virSocketAddrPtr addr, +int virSocketAddrBroadcastByPrefix(const virSocketAddr *addr, unsigned int prefix, virSocketAddrPtr broadcast); diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 0d415df..ca6e2b5 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -472,7 +472,7 @@ virSocketAddrMaskByPrefix(const virSocketAddr *addr, * Returns 0 in case of success, or -1 on error. */ int -virSocketAddrBroadcast(const virSocketAddrPtr addr, +virSocketAddrBroadcast(const virSocketAddr *addr, const virSocketAddrPtr netmask, virSocketAddrPtr broadcast) { @@ -502,7 +502,7 @@ virSocketAddrBroadcast(const virSocketAddrPtr addr, * Returns 0 in case of success, or -1 on error. */ int -virSocketAddrBroadcastByPrefix(const virSocketAddrPtr addr, +virSocketAddrBroadcastByPrefix(const virSocketAddr *addr, unsigned int prefix, virSocketAddrPtr broadcast) { -- 1.8.3.1
Packets sent by guests on virbrN, *or* by dnsmasq on the same, to - 255.255.255.255/32 (netmask-independent local network broadcast address), or to - eg. 192.168.122.255/32 (ie. address- and netmask-dependent (= directed) local network broadcast address), or to - 224.0.0.0/24 (local subnetwork multicast range) are never forwarded, hence it is not necessary to masquerade them. In fact we must not masquerade them: translating their source addresses or source ports (where applicable) may confuse receivers on virbrN. One example is the DHCP client in OVMF (= UEFI firmware for virtual machines): http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1506/focus=2640 It expects DHCP replies to arrive from remote source port 67. Even though dnsmasq conforms to that, the destination address (255.255.255.255) and the source address (eg. 192.168.122.1) in the reply allow the UDP masquerading rule to match, which rewrites the source port to or above 1024. This prevents the DHCP client in OVMF from accepting the packet. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=709418 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- v2->v3: - also prevent masquerading of directed broadcast [Laine] src/network/bridge_driver_linux.c | 151 +++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 80430a8..093cba1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -127,11 +127,64 @@ out: return ret; } +/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions: + * + * 1. do not masquerade packets targeting 224.0.0.0/24 + * 2. do not masquerade packets targeting 255.255.255.255/32 + * 3. do not masquerade packets targeting the directed local broadcast + * address + * + * 224.0.0.0/24 is the local network multicast range. Packets are not + * forwarded outside. + * + * 255.255.255.255/32 is the broadcast address of any local network. Again, + * such packets are never forwarded, but strict DHCP clients don't accept + * DHCP replies with changed source ports. + * + * The directed local broadcast address looks like 192.168.122.255/32, and + * behaves like the generic broadcast address 255.255.255.255/32. + * + * Returns 0 on success and -1 on failure. + */ +static int networkFillMasqExceptions(const char *bridgeName, + const virPfxSocketAddr *bridge, + virPfxSocketAddr *localMulticast, + virPfxSocketAddr *genericBroadcast, + virPfxSocketAddr *directedBroadcast) +{ + int result; + + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1); + + genericBroadcast->prefix = 32; + result = virSocketAddrParseIPv4(&genericBroadcast->addr, + "255.255.255.255"); + sa_assert(result != -1); + + directedBroadcast->prefix = 32; + result = virSocketAddrBroadcastByPrefix(&bridge->addr, bridge->prefix, + &directedBroadcast->addr); + if (result == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Couldn't form directed broadcast address for '%s'"), + bridgeName); + return -1; + } + return 0; +} + int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast; if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -140,6 +193,15 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr1; } + bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == -1) + goto masqerr1; + /* allow forwarding packets from the bridge interface */ if (iptablesAddForwardAllowOut(&ipdef->address, prefix, @@ -167,11 +229,12 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, /* * Enable masquerading. * - * We need to end up with 3 rules in the table in this order + * We need to end up with 6 rules in the table in this order * - * 1. protocol=tcp with sport mapping restriction - * 2. protocol=udp with sport mapping restriction - * 3. generic any protocol + * 1-3. masquerading exceptions + * 4. masquerade protocol=tcp with sport mapping restriction + * 5. masquerade protocol=udp with sport mapping restriction + * 6. generic, masquerade any protocol * * The sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -238,8 +301,65 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr5; } + /* exempt generic broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent generic broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent generic broadcast masquerading")); + goto masqerr6; + } + + /* exempt directed broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent directed broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent directed broadcast masquerading")); + goto masqerr7; + } + + /* exempt local multicast range as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent local multicast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent local multicast masquerading")); + goto masqerr8; + } + return 0; + masqerr8: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + masqerr7: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + masqerr6: + iptablesRemoveForwardMasquerade(&ipdef->address, + prefix, + forwardIf, + &network->def->forward.addr, + &network->def->forward.port, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(&ipdef->address, prefix, @@ -275,6 +395,29 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, const char *forwardIf = virNetworkDefForwardIf(network->def, 0); if (prefix >= 0) { + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast; + + bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == 0) { + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + } + iptablesRemoveForwardMasquerade(&ipdef->address, prefix, forwardIf, -- 1.8.3.1
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
Packets sent by guests on virbrN, *or* by dnsmasq on the same, to - 255.255.255.255/32 (netmask-independent local network broadcast address), or to - eg. 192.168.122.255/32 (ie. address- and netmask-dependent (= directed) local network broadcast address)
As you pointed out in an email responding to my request for this - it's not necessary as it is already covered by all of the rules that *do* want masquerading being qualified with "! -d 192.168.122.0/24" (for example). So you can/should take it out. Sorry for creating the extra work.
or to - 224.0.0.0/24 (local subnetwork multicast range) are never forwarded, hence it is not necessary to masquerade them.
In fact we must not masquerade them: translating their source addresses or source ports (where applicable) may confuse receivers on virbrN.
One example is the DHCP client in OVMF (= UEFI firmware for virtual machines):
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1506/focus=2640
It expects DHCP replies to arrive from remote source port 67. Even though dnsmasq conforms to that, the destination address (255.255.255.255) and the source address (eg. 192.168.122.1) in the reply allow the UDP masquerading rule to match, which rewrites the source port to or above 1024. This prevents the DHCP client in OVMF from accepting the packet.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=709418
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- v2->v3: - also prevent masquerading of directed broadcast [Laine]
src/network/bridge_driver_linux.c | 151 +++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 4 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 80430a8..093cba1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -127,11 +127,64 @@ out: return ret; }
+/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions: + * + * 1. do not masquerade packets targeting 224.0.0.0/24 + * 2. do not masquerade packets targeting 255.255.255.255/32 + * 3. do not masquerade packets targeting the directed local broadcast + * address + * + * 224.0.0.0/24 is the local network multicast range. Packets are not + * forwarded outside. + * + * 255.255.255.255/32 is the broadcast address of any local network. Again, + * such packets are never forwarded, but strict DHCP clients don't accept + * DHCP replies with changed source ports. + * + * The directed local broadcast address looks like 192.168.122.255/32, and + * behaves like the generic broadcast address 255.255.255.255/32. + * + * Returns 0 on success and -1 on failure. + */ +static int networkFillMasqExceptions(const char *bridgeName, + const virPfxSocketAddr *bridge, + virPfxSocketAddr *localMulticast, + virPfxSocketAddr *genericBroadcast, + virPfxSocketAddr *directedBroadcast) +{ + int result; + + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1);
You must have accidentally left this in. libvirt is a library, so it must never assert. In a case where the called function is guaranteed to never fail (due to the args passed in), you can enclose it in ignore_value(): ignore_value(cirSocketAddrParseIPv4(.......)
+ + genericBroadcast->prefix = 32; + result = virSocketAddrParseIPv4(&genericBroadcast->addr, + "255.255.255.255"); + sa_assert(result != -1); + + directedBroadcast->prefix = 32; + result = virSocketAddrBroadcastByPrefix(&bridge->addr, bridge->prefix, + &directedBroadcast->addr); + if (result == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Couldn't form directed broadcast address for '%s'"), + bridgeName); + return -1; + } + return 0; +} + int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast;
if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -140,6 +193,15 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr1; }
+ bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == -1) + goto masqerr1; + /* allow forwarding packets from the bridge interface */ if (iptablesAddForwardAllowOut(&ipdef->address, prefix, @@ -167,11 +229,12 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, /* * Enable masquerading. * - * We need to end up with 3 rules in the table in this order + * We need to end up with 6 rules in the table in this order * - * 1. protocol=tcp with sport mapping restriction - * 2. protocol=udp with sport mapping restriction - * 3. generic any protocol + * 1-3. masquerading exceptions + * 4. masquerade protocol=tcp with sport mapping restriction + * 5. masquerade protocol=udp with sport mapping restriction + * 6. generic, masquerade any protocol * * The sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -238,8 +301,65 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr5; }
+ /* exempt generic broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent generic broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent generic broadcast masquerading")); + goto masqerr6; + } + + /* exempt directed broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent directed broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent directed broadcast masquerading")); + goto masqerr7; + } + + /* exempt local multicast range as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent local multicast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent local multicast masquerading")); + goto masqerr8; + } + return 0;
+ masqerr8: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + masqerr7: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + masqerr6: + iptablesRemoveForwardMasquerade(&ipdef->address, + prefix, + forwardIf, + &network->def->forward.addr, + &network->def->forward.port, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(&ipdef->address, prefix, @@ -275,6 +395,29 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
if (prefix >= 0) { + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast; + + bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == 0) { + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + } + iptablesRemoveForwardMasquerade(&ipdef->address, prefix, forwardIf,
ACK once the ill-fated directed broadcast rule and sa_asserts are removed.
On 09/24/13 11:31, Laine Stump wrote:
On 09/23/2013 08:03 PM, Laszlo Ersek wrote:
+/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions: + * + * 1. do not masquerade packets targeting 224.0.0.0/24 + * 2. do not masquerade packets targeting 255.255.255.255/32 + * 3. do not masquerade packets targeting the directed local broadcast + * address + * + * 224.0.0.0/24 is the local network multicast range. Packets are not + * forwarded outside. + * + * 255.255.255.255/32 is the broadcast address of any local network. Again, + * such packets are never forwarded, but strict DHCP clients don't accept + * DHCP replies with changed source ports. + * + * The directed local broadcast address looks like 192.168.122.255/32, and + * behaves like the generic broadcast address 255.255.255.255/32. + * + * Returns 0 on success and -1 on failure. + */ +static int networkFillMasqExceptions(const char *bridgeName, + const virPfxSocketAddr *bridge, + virPfxSocketAddr *localMulticast, + virPfxSocketAddr *genericBroadcast, + virPfxSocketAddr *directedBroadcast) +{ + int result; + + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1);
You must have accidentally left this in. libvirt is a library, so it must never assert. In a case where the called function is guaranteed to never fail (due to the args passed in), you can enclose it in ignore_value():
ignore_value(cirSocketAddrParseIPv4(.......)
Ah. Good to know! In fact I had searched the HACKING file for "assert", and there were no hits. So I grepped the source :) (BTW there *are* some sa_assert() calls in eg. "src/qemu/qemu_driver.c". And, as far as I saw, sa_assert() expands to /* empty */ unless STATIC_ANALYSIS is defined. I kind of didn't understand that, actually; but I found no naked "assert()" calls in the source. Now that you say that a library is never supposed to assert() -- I'm not sure I agree with that FWIW :) -- it makes sense.) Thanks Laszlo
On 09/24/2013 07:23 AM, Laszlo Ersek wrote:
+ + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1);
You must have accidentally left this in. libvirt is a library, so it must never assert. In a case where the called function is guaranteed to never fail (due to the args passed in), you can enclose it in ignore_value():
Libraries must NOT use assert(). But libvirt MAY use sa_assert() - which exists only as a hint to shut up puny static analyzers and NOT as a way to abort execution if the constraint is violated (of course, if the constraint is violated, we still have a bug that needs fixing...).
ignore_value(cirSocketAddrParseIPv4(.......)
Ah. Good to know!
In fact I had searched the HACKING file for "assert", and there were no hits. So I grepped the source :)
We have uses of assert() in tools/virsh*.c, but only because virsh is an end-user executable, and not a library. But you are correct that we must NOT have it within daemon/ or src/. And indeed, ignore_value() is a nice way to declare that we know our particular call isn't worth checking for failure. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/24/13 18:28, Eric Blake wrote:
On 09/24/2013 07:23 AM, Laszlo Ersek wrote:
+ + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1);
You must have accidentally left this in. libvirt is a library, so it must never assert. In a case where the called function is guaranteed to never fail (due to the args passed in), you can enclose it in ignore_value():
Libraries must NOT use assert(). But libvirt MAY use sa_assert() - which exists only as a hint to shut up puny static analyzers and NOT as a way to abort execution if the constraint is violated (of course, if the constraint is violated, we still have a bug that needs fixing...).
OK this is theoretical now, but: what do you do when you detect corruption of internal state? Ie. violation of invariants that are the basis of whatever you do in the library? Going forward can easily exacerbate the damage. In life-critical systems you shut down (or isolate) the faulty component I guess and let the redundant component(s) take over. But I think a normal library is allowed to assert(), same as the kernel is allowed to call BUG_ON(). Laszlo
On 09/24/13 02:03, Laszlo Ersek wrote:
v2->v3 changes: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine]. - Pass (address, prefix) pairs as both source and destination parameters to these functions. - Introduce virPfxSocketAddr structure for simpler handling of said (address, prefix) pairs. - Also prevent masquerading of directed broadcast [Laine]. - Start to get serious about pointers-to-const.
OK, let me summarize the comments still standing: For v2: - Laine wants the functions added in patch #1 renamed. http://thread.gmane.org/gmane.comp.emulators.libvirt/85709/focus=85715 For v3: - Missing address family check for @dst in iptablesDontMasquerade() in patch #2 [Laine] http://thread.gmane.org/gmane.comp.emulators.libvirt/85751/focus=85772 - Drop the sa_assert()s in networkFillMasqExceptions() in patch #4 [Laine] http://thread.gmane.org/gmane.comp.emulators.libvirt/85751/focus=85774 - Drop the address-dependent broadcast rule in patch #4 [Laine] same message The address-dependent broadcast rule in patch #4 (that couldn't be hard-coded) was the reason for all of the new code between v2 and v3. If I drop that iptables rule, but keep the rest of v3, I'll be thrashing a bunch of code around for no good reason. I might as well fix up v2 as requested originally, and submit that as v4. What do you recommend? I think fixing up v2 with the renames is a better approach. I'm fine either way, I'd just like to get this merged and stop wasting the time of y'all. Thanks! Laszlo
On 09/24/2013 02:38 PM, Laszlo Ersek wrote:
I might as well fix up v2 as requested originally, and submit that as v4.
What do you recommend? I think fixing up v2 with the renames is a better approach. I'm fine either way, I'd just like to get this merged and stop wasting the time of y'all.
That sounds fine to me. Again, sorry for causing much of the unnecessary churn.
On 09/25/13 09:22, Laine Stump wrote:
On 09/24/2013 02:38 PM, Laszlo Ersek wrote:
I might as well fix up v2 as requested originally, and submit that as v4.
What do you recommend? I think fixing up v2 with the renames is a better approach. I'm fine either way, I'd just like to get this merged and stop wasting the time of y'all.
That sounds fine to me. Again, sorry for causing much of the unnecessary churn.
No problem. Thanks! Laszlo
participants (5)
-
Eric Blake -
Laine Stump -
Laine Stump -
Laszlo Ersek -
Paolo Bonzini