On 09/25/13 12:54, Laine Stump wrote: > 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@xxxxxxxxxx> >> --- >> 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. I think your comments have all been to the point -- I never had the impression of nit-picking. Thank you very much! Laszlo -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list