... 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(a)redhat.com&gt; --- 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/24/2013 07:33 AM, Laszlo Ersek wrote: 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

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(a)redhat.com&gt; --- 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/24/13 11:24, Laine Stump wrote: 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. 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. 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

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(a)redhat.com&gt; --- 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