Re: [libvirt] [PATCH v4 2/2] bridge driver: don't masquerade local subnet broadcast/multicast packets

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