[libvirt] [PATCH] BZ 657918 Default iptables setup in libvirt breaks mDNS
Hi, Per the request on https://bugzilla.redhat.com/show_bug.cgi?id=657918 please find attached a patch that should address the issue. I'm not subscribed to this list though (I know, it's pretty rude, but my e-mail traffic is already too heavy to add another list to it), so if you could either CC me on any follow-up or just move followups to the BZ ticket where the patch also appears, that would be great. Cheers, b. --- src/network/bridge_driver.c.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/network/bridge_driver.c 2012-12-11 15:49:13.937133883 -0500 @@ -1301,9 +1301,10 @@ * * We need to end up with 3 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. multicast is exempted + * 2. protocol=tcp with sport mapping restriction + * 3. protocol=udp with sport mapping restriction + * 4. generic any protocol * * The sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -1361,8 +1362,21 @@ goto masqerr5; } + /* exempt multicast traffic */ + if (iptablesAddForwardMasqueradeExempt(driver->iptables) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to exempt multicast traffic from masquerading")); + goto masqerr6; + } + return 0; + masqerr6: + iptablesRemoveForwardMasquerade(driver->iptables, + &ipdef->address, + prefix, + forwardIf, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, --- src/util/iptables.c.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/util/iptables.c 2012-12-11 15:53:28.715044866 -0500 @@ -858,6 +858,26 @@ } /** + * iptablesAddForwardMasqueradeExempt: + * @ctx: pointer to the IP table context + * + * Add rules to the IP table context to exempt masquerading + * for multicast networks + * + * Returns 0 in case of success or an error code otherwise + */ +int +iptablesAddForwardMasqueradeExempt(iptablesContext *ctx) +{ + return iptablesAddRemoveRule(ctx->nat_postrouting, + AF_INET, + ADD, + "--destination", "224.0.0.0/4", + "--jump", "RETURN", + NULL); +} + +/** * iptablesAddForwardMasquerade: * @ctx: pointer to the IP table context * @network: the source network name --- src/util/iptables.h.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/util/iptables.h 2012-12-11 15:57:03.284144679 -0500 @@ -101,6 +101,7 @@ int family, const char *iface); +int iptablesAddForwardMasqueradeExempt (iptablesContext *ctx); int iptablesAddForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, --- src/libvirt_private.syms.orig 2012-12-11 15:46:11.141932324 -0500 +++ src/libvirt_private.syms 2012-12-11 15:58:11.715865516 -0500 @@ -681,6 +681,7 @@ iptablesAddForwardAllowOut; iptablesAddForwardAllowRelatedIn; iptablesAddForwardMasquerade; +iptablesAddForwardMasqueradeExempt; iptablesAddForwardRejectIn; iptablesAddForwardRejectOut; iptablesAddOutputFixUdpChecksum;
On 12/11/2012 03:54 PM, Brian J. Murrell wrote:
Hi,
Per the request on https://bugzilla.redhat.com/show_bug.cgi?id=657918 please find attached a patch that should address the issue.
Thanks!
I'm not subscribed to this list though (I know, it's pretty rude, but my e-mail traffic is already too heavy to add another list to it), so if you could either CC me on any follow-up or just move followups to the BZ ticket where the patch also appears, that would be great.
Don't worry about it. List policy is to reply-to-all precisely so that you do not have to subscribe while still following a thread you initiate (and mail-followup-to exists for those who do subscribe but don't like duplicates). I'll comment on the minor coding issues, but hopefully someone more familiar with actual filtering can chime in on the logical correctness.
Cheers, b.
--- src/network/bridge_driver.c.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/network/bridge_driver.c 2012-12-11 15:49:13.937133883 -0500 @@ -1301,9 +1301,10 @@ * * We need to end up with 3 rules in the table in this order
Comment should now mention 4 rules.
* - * 1. protocol=tcp with sport mapping restriction - * 2. protocol=udp with sport mapping restriction - * 3. generic any protocol + * 1. multicast is exempted + * 2. protocol=tcp with sport mapping restriction + * 3. protocol=udp with sport mapping restriction + * 4. generic any protocol * * The sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -1361,8 +1362,21 @@ goto masqerr5; }
+ /* exempt multicast traffic */ + if (iptablesAddForwardMasqueradeExempt(driver->iptables) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to exempt multicast traffic from masquerading"));
Indentation is a bit off, and you need a "%s" argument to keep the syntax-checker happy about a message with no other % operand.
+ goto masqerr6; + } + return 0;
+ masqerr6: + iptablesRemoveForwardMasquerade(driver->iptables, + &ipdef->address, + prefix, + forwardIf, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, --- src/util/iptables.c.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/util/iptables.c 2012-12-11 15:53:28.715044866 -0500 @@ -858,6 +858,26 @@ }
/** + * iptablesAddForwardMasqueradeExempt: + * @ctx: pointer to the IP table context + * + * Add rules to the IP table context to exempt masquerading + * for multicast networks + * + * Returns 0 in case of success or an error code otherwise + */ +int +iptablesAddForwardMasqueradeExempt(iptablesContext *ctx) +{ + return iptablesAddRemoveRule(ctx->nat_postrouting, + AF_INET, + ADD, + "--destination", "224.0.0.0/4", + "--jump", "RETURN", + NULL); +}
Do we need an IPv6 counterpart? (Or am I just showing my ignorance of what IPv6 does as a counterpart to IPv4 multicast?)
+ +/** * iptablesAddForwardMasquerade: * @ctx: pointer to the IP table context * @network: the source network name --- src/util/iptables.h.orig 2012-10-27 16:56:23.000000000 -0400 +++ src/util/iptables.h 2012-12-11 15:57:03.284144679 -0500 @@ -101,6 +101,7 @@ int family, const char *iface);
+int iptablesAddForwardMasqueradeExempt (iptablesContext *ctx); int iptablesAddForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, --- src/libvirt_private.syms.orig 2012-12-11 15:46:11.141932324 -0500 +++ src/libvirt_private.syms 2012-12-11 15:58:11.715865516 -0500 @@ -681,6 +681,7 @@ iptablesAddForwardAllowOut; iptablesAddForwardAllowRelatedIn; iptablesAddForwardMasquerade; +iptablesAddForwardMasqueradeExempt; iptablesAddForwardRejectIn; iptablesAddForwardRejectOut; iptablesAddOutputFixUdpChecksum;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12-12-11 06:24 PM, Eric Blake wrote:
Thanks!
NP. Had it just lying around here anyway. :-)
Comment should now mention 4 rules.
Doh! Missed that in the patch port. Updated in my local copy (which I will of course resend once all of the initial review is done).
+ /* exempt multicast traffic */ + if (iptablesAddForwardMasqueradeExempt(driver->iptables) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to exempt multicast traffic from masquerading"));
Indentation is a bit off,
OK. Fixed (again, locally).
and you need a "%s" argument to keep the syntax-checker happy about a message with no other % operand.
Hrm. There is no argument to substitute into a %s though. There appear to be lots of other "virReportError()" calls with no %s in them if there is no argument such as: virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? _("failed to add iptables rule to enable masquerading to %s") : _("failed to add iptables rule to enable masquerading"), forwardIf); Notice if forwardIf is NULL, it will use the: _("failed to add iptables rule to enable masquerading"), branch. Of course I could be missing something.
Do we need an IPv6 counterpart? (Or am I just showing my ignorance of what IPv6 does as a counterpart to IPv4 multicast?)
Hrm. I wouldn't think so. NAT (which is what masquerading is) isn't supposed to exist in IPv6. Billions of addresses and all that. :-) Unless my understanding is incorrect that is. Cheers, b.
On 12/11/2012 07:05 PM, Brian J. Murrell wrote:
On 12-12-11 06:24 PM, Eric Blake wrote:
Thanks! NP. Had it just lying around here anyway. :-)
Comment should now mention 4 rules. Doh! Missed that in the patch port. Updated in my local copy (which I will of course resend once all of the initial review is done).
The one thing I would ask beyond Eric's suggestions is that you use git send-email to produce the patches - the patch you've sent doesn't apply with git am, which would make it a pain to properly attribute to you. If you're unfamiliar with using git, here's what you would do: 1) git clone git://libvirt.org/libvirt.git 2) cd libvirt 3) edit the files. 4) make check && make syntax-check (and fix any problems they find) 4) git add $list-of-files 5) git commit (give a nice descriptive log message with subject of "network: support mDNS on NAT networks") 6) git send-email -1 (tell it to send to libvir-list@redhat.com) I can then directly apply the patch with git am.
+ /* exempt multicast traffic */ + if (iptablesAddForwardMasqueradeExempt(driver->iptables) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to exempt multicast traffic from masquerading")); Indentation is a bit off, OK. Fixed (again, locally).
and you need a "%s" argument to keep the syntax-checker happy about a message with no other % operand. Hrm. There is no argument to substitute into a %s though. There appear to be lots of other "virReportError()" calls with no %s in them if there is no argument such as:
virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? _("failed to add iptables rule to enable masquerading to %s") : _("failed to add iptables rule to enable masquerading"), forwardIf);
Notice if forwardIf is NULL, it will use the:
_("failed to add iptables rule to enable masquerading"),
branch. Of course I could be missing something.
I'm surprised that doesn't generate a compile error, other cases of _() with no %whatever will. It must be the ?: that's messing up the compiler's checking. At any rate, that should be fixed (separate from your patch though, of course)
Do we need an IPv6 counterpart? (Or am I just showing my ignorance of what IPv6 does as a counterpart to IPv4 multicast?) Hrm. I wouldn't think so. NAT (which is what masquerading is) isn't supposed to exist in IPv6. Billions of addresses and all that. :-) Unless my understanding is incorrect that is.
Well, there is now at least a proposal for some sort of IPv6 NAT, but libvirt networks only do routed IPv6 networks, so it shouldn't be necessary.
On 12/12/2012 01:19 PM, Laine Stump wrote:
On 12/11/2012 07:05 PM, Brian J. Murrell wrote:
On 12-12-11 06:24 PM, Eric Blake wrote:
Thanks! NP. Had it just lying around here anyway. :-)
Comment should now mention 4 rules. Doh! Missed that in the patch port. Updated in my local copy (which I will of course resend once all of the initial review is done).
The one thing I would ask beyond Eric's suggestions is that you use git send-email to produce the patches - the patch you've sent doesn't apply with git am, which would make it a pain to properly attribute to you.
If you're unfamiliar with using git, here's what you would do:
1) git clone git://libvirt.org/libvirt.git
2) cd libvirt
3) edit the files.
4) make check && make syntax-check (and fix any problems they find)
4) git add $list-of-files
5) git commit
(give a nice descriptive log message with subject of "network: support mDNS on NAT networks")
6) git send-email -1
(tell it to send to libvir-list@redhat.com)
I can then directly apply the patch with git am.
I noticed this hadn't been applied yet, and was wondering if you were still planning on resubmitting this patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 13-01-08 11:59 AM, Eric Blake wrote:
I noticed this hadn't been applied yet, and was wondering if you were still planning on resubmitting this patch.
Yeah, I'd definitely like to. It's just not bubbled back up to the top of the TODO list again yet. Hopefully very soon. Cheers, b.
participants (3)
-
Brian J. Murrell -
Eric Blake -
Laine Stump