On 09/25/2013 06:45 AM, Laszlo Ersek wrote:

The functions - iptablesAddForwardDontMasquerade(), - iptablesRemoveForwardDontMasquerade 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.

ACK, with the small change to to note the new function names in the commit log message as well. I'll fix that before I push it.

Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- v2->v4: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine].

src/util/viriptables.h | 8 +++++ src/util/viriptables.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 98 insertions(+)

diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 447f4a8..6dfb992 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -94,6 +94,14 @@ int iptablesRemoveForwardMasquerade (virSocketAddr *netaddr, virSocketAddrRangePtr addr, virPortRangePtr port, const char *protocol); +int iptablesAddDontMasquerade (virSocketAddr *netaddr, + unsigned int prefix, + const char *physdev, + const char *destaddr); +int iptablesRemoveDontMasquerade (virSocketAddr *netaddr, + unsigned int prefix, + const char *physdev, + const char *destaddr); 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 0f57d66..16f571e 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -832,6 +832,94 @@ iptablesRemoveForwardMasquerade(virSocketAddr *netaddr, }

+/* Don't masquerade traffic coming from the network associated with the bridge + * if said traffic targets @destaddr. + */ +static int +iptablesForwardDontMasquerade(virSocketAddr *netaddr, + unsigned int prefix, + const char *physdev, + const char *destaddr, + int action) +{ + int ret = -1; + char *networkstr = NULL; + virCommandPtr cmd = NULL; + + if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) + return -1; + + if (!VIR_SOCKET_ADDR_IS_FAMILY(netaddr, 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."), + networkstr); + goto cleanup; + } + + cmd = iptablesCommandNew("nat", "POSTROUTING", AF_INET, action); + + if (physdev && physdev[0]) + virCommandAddArgList(cmd, "--out-interface", physdev, NULL); + + virCommandAddArgList(cmd, "--source", networkstr, + "--destination", destaddr, "--jump", "RETURN", NULL); + ret = virCommandRun(cmd, NULL); +cleanup: + virCommandFree(cmd); + VIR_FREE(networkstr); + return ret; +} + +/** + * iptablesAddDontMasquerade: + * @netaddr: the source network name + * @prefix: prefix (# of 1 bits) of netmask to apply to @netaddr + * @physdev: the physical output device or NULL + * @destaddr: the destination network not to masquerade for + * + * Add rules to the IP table context to avoid masquerading from + * @netaddr/@prefix to @destaddr on @physdev. @destaddr must be in a format + * directly consumable by iptables, it must not depend on user input or + * configuration. + * + * Returns 0 in case of success or an error code otherwise. + */ +int +iptablesAddDontMasquerade(virSocketAddr *netaddr, + unsigned int prefix, + const char *physdev, + const char *destaddr) +{ + return iptablesForwardDontMasquerade(netaddr, prefix, physdev, destaddr, + ADD); +} + +/** + * iptablesRemoveDontMasquerade: + * @netaddr: the source network name + * @prefix: prefix (# of 1 bits) of netmask to apply to @netaddr + * @physdev: the physical output device or NULL + * @destaddr: the destination network not to masquerade for + * + * Remove rules from the IP table context that prevent masquerading from + * @netaddr/@prefix to @destaddr on @physdev. @destaddr must be in a format + * directly consumable by iptables, it must not depend on user input or + * configuration. + * + * Returns 0 in case of success or an error code otherwise. + */ +int +iptablesRemoveDontMasquerade(virSocketAddr *netaddr, + unsigned int prefix, + const char *physdev, + const char *destaddr) +{ + return iptablesForwardDontMasquerade(netaddr, prefix, physdev, destaddr, + REMOVE); +} + + static int iptablesOutputFixUdpChecksum(const char *iface, int port, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1ecee4c..48e4b04 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1448,6 +1448,7 @@ virInitctlSetRunLevel;

# util/viriptables.h +iptablesAddDontMasquerade; iptablesAddForwardAllowCross; iptablesAddForwardAllowIn; iptablesAddForwardAllowOut; @@ -1458,6 +1459,7 @@ iptablesAddForwardRejectOut; iptablesAddOutputFixUdpChecksum; iptablesAddTcpInput; iptablesAddUdpInput; +iptablesRemoveDontMasquerade; iptablesRemoveForwardAllowCross; iptablesRemoveForwardAllowIn; iptablesRemoveForwardAllowOut;

On 09/25/2013 06:45 AM, 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 - 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->v4: - Rename iptables(Add|Remove)ForwardDontMasquerade to iptables(Add|Remove)DontMasquerade [Laine].

src/network/bridge_driver_linux.c | 70 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 80430a8..066779a 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -127,6 +127,9 @@ out: return ret; }

+static const char networkLocalMulticast[] = "224.0.0.0/24"; +static const char networkLocalBroadcast[] = "255.255.255.255/32"; + int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, virNetworkIpDefPtr ipdef) { @@ -167,11 +170,20 @@ 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 5 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. do not masquerade packets targeting 224.0.0.0/24 + * 2. do not masquerade packets targeting 255.255.255.255/32 + * 3. masquerade protocol=tcp with sport mapping restriction + * 4. masquerade protocol=udp with sport mapping restriction + * 5. generic, masquerade any protocol + * + * 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 sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -238,8 +250,50 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr5; }

+ /* exempt local network broadcast address as destination */ + if (iptablesAddDontMasquerade(&ipdef->address, + prefix, + forwardIf, + networkLocalBroadcast) < 0) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent local broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent local broadcast masquerading")); + goto masqerr6; + } + + /* exempt local multicast range as destination */ + if (iptablesAddDontMasquerade(&ipdef->address, + prefix, + forwardIf, + networkLocalMulticast) < 0) { + 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 masqerr7; + } + return 0;

+ masqerr7: + iptablesRemoveDontMasquerade(&ipdef->address, + prefix, + forwardIf, + networkLocalBroadcast); + masqerr6: + iptablesRemoveForwardMasquerade(&ipdef->address, + prefix, + forwardIf, + &network->def->forward.addr, + &network->def->forward.port, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(&ipdef->address, prefix, @@ -275,6 +329,14 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, const char *forwardIf = virNetworkDefForwardIf(network->def, 0);

if (prefix >= 0) { + iptablesRemoveDontMasquerade(&ipdef->address, + prefix, + forwardIf, + networkLocalMulticast); + iptablesRemoveDontMasquerade(&ipdef->address, + prefix, + forwardIf, + networkLocalBroadcast); iptablesRemoveForwardMasquerade(&ipdef->address, prefix, forwardIf,

ACK. I'll push both patches as soon as I've done a sanity build locally. Thanks for putting up with the nit-picking and false direction.