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

[Laszlo Ersek]

On 09/23/13 16:40, Brian J. Murrell wrote:
> On 13-09-23 10:05 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)
> 
> All multicast, not just the local subnet multicast needs to be exempt
> from masquerading.
> 
> I would tend to guess that anyone trying to do "global Internet"
> multicast behind NAT is signing up for a world of trouble anyway.  It's
> not like you could put a multicast source behind a NAT so it could
> really only apply to sinks.
> 
> Since sinks operate by "subscribing" to a multicast source by way of
> their multicast router (which should be local and not through a NAT) I
> would think a NAT device that supports multicast to it's NAT clients
> should support it as if it were a multicast router and not so much a NAT
> and so there never really should be NATting of traffic from a sink.
> 
> So AFACT there never really is a use-case for actually NATting multicast
> traffic, so just don't NAT the whole range, 224.0.0.0/4.
> 
>> 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):
> 
> An example use-case if you want for multicast is creating a corosync
> cluster.  When that works, you have this patch right.  :-)
> 
>> diff --git a/src/network/bridge_driver_linux.c
>> b/src/network/bridge_driver_linux.c
>> index 80430a8..95add0e 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";
> 
> NACK.  224.0.0.0/4 is the entire multicast space.  224.0.0.0/24 is just
> one tiny reserved "control block" of addresses for routing protocols,
> etc.  There's a lot more to multicast than just routing protocols.

I won't do that in this series (see also Laine's review).

In

  https://bugzilla.redhat.com/show_bug.cgi?id=709418#c13

I suggested to go with "224.0.0.0/24" now, and to let someone with good
knowledge of multicast implement "224.0.0.0/4". You could write that
patch too, and this email of yours would certainly be a good commit
message basis.

If you disagree with this approach (that is: if you think that
"224.0.0.0/24" here is not gradual improvement but a step in the wrong
direction), then
- I'll drop "networkLocalMulticast" plus what depends on it completely,
- I'll drop the BZ reference from the commit message,
- I'll retitle the BZ so that it clearly concerns multicast only, again,
like it did when you submitted it,
- in short my series will focus only on local broadcast.

The broadcast and the multicast use cases are distinct. I only tried to
merge them because they appeared to affect the same code -- what I
perceived to be a good first step for multicast seemed to be
implementable with the same effort as local broadcast. However, if you
think we can't (or shouldn't) build on this "first multicast step"
present in the patch, then I'll drop multicast altogether.

Sound good?

Laszlo