[libvirt] [PATCH 1/5] reload iptables rules simply by re-adding them

Mark McLoughlin

10 Dec 2009 10 Dec '09

12:27 p.m.

Currently, when we add iptables rules, we keep them on a list so that we can easily reload them on e.g. 'service libvirtd reload'. However, we don't save this list to disk, so if libvirtd is restarted we lose the ability to reload the rules. The fix is simple - just re-add the damn things on reload. Note, we delete the rules before re-adding them, just like the current behaviour of iptRulesReload(). * src/network/bridge_driver.c: re-add the iptables rules on reload. --- src/network/bridge_driver.c | 30 ++++++++++++++++++++++++------ 1 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0342aa0..766f8cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -96,6 +96,8 @@ static int networkShutdownNetworkDaemon(virConnectPtr conn, struct network_driver *driver, virNetworkObjPtr network); +static void networkReloadIptablesRules(struct network_driver *driver); + static struct network_driver *driverState = NULL; @@ -291,12 +293,7 @@ networkReload(void) { &driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir); - - if (driverState->iptables) { - VIR_INFO0(_("Reloading iptables rules\n")); - iptablesReloadRules(driverState->iptables); - } - + networkReloadIptablesRules(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); return 0; @@ -812,6 +809,27 @@ networkRemoveIptablesRules(struct network_driver *driver, iptablesSaveRules(driver->iptables); } +static void +networkReloadIptablesRules(struct network_driver *driver) +{ + unsigned int i; + + VIR_INFO0(_("Reloading iptables rules")); + + for (i = 0 ; i < driver->networks.count ; i++) { + virNetworkObjLock(driver->networks.objs[i]); + + if (virNetworkObjIsActive(driver->networks.objs[i])) { + networkRemoveIptablesRules(driver, driver->networks.objs[i]); + if (!networkAddIptablesRules(NULL, driver, driver->networks.objs[i])) { + /* failed to add but already logged */ + } + } + + virNetworkObjUnlock(driver->networks.objs[i]); + } +} + /* Enable IP Forwarding. Return 0 for success, -1 for failure. */ static int networkEnableIpForwarding(void) -- 1.6.5.2

Show replies by date

Mark McLoughlin

10 Dec 10 Dec

12:27 p.m.

New subject: [libvirt] [PATCH 2/5] reload iptables rules on libvirtd restart

This is the expected behaviour, I think - reloading libvirtd should be a subset of restarting it. Note, we reload the rules after we've determined which networks are active (because we only add the rules for active networks) and before we start autostart networks (to avoid re-adding the rules). * src/network/bridge_driver.c: reload iptables rules on startup --- src/network/bridge_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 766f8cd..d5cab71 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -259,6 +259,7 @@ networkStartup(int privileged) { goto error; networkFindActiveConfigs(driverState); + networkReloadIptablesRules(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); -- 1.6.5.2

Daniel P. Berrange

1:04 p.m.

New subject: [libvirt] [PATCH 2/5] reload iptables rules on libvirtd restart

On Thu, Dec 10, 2009 at 11:27:52AM +0000, Mark McLoughlin wrote:

...

This is the expected behaviour, I think - reloading libvirtd should be a subset of restarting it.

Note, we reload the rules after we've determined which networks are active (because we only add the rules for active networks) and before we start autostart networks (to avoid re-adding the rules).

* src/network/bridge_driver.c: reload iptables rules on startup --- src/network/bridge_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 766f8cd..d5cab71 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -259,6 +259,7 @@ networkStartup(int privileged) { goto error;

networkFindActiveConfigs(driverState); + networkReloadIptablesRules(driverState); networkAutostartConfigs(driverState);

networkDriverUnlock(driverState);

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Mark McLoughlin

12:27 p.m.

New subject: [libvirt] [PATCH 3/5] remove all traces of lokkit support

Long ago we tried to use Fedora's lokkit utility in order to register our iptables rules so that 'service iptables restart' would automatically load our rules. There was one fatal flaw - if the user had configured iptables without lokkit, then we would clobber that configuration by running lokkit. We quickly disabled lokkit support, but never removed it. Let's do that now. The 'my virtual network stops working when I restart iptables' still remains. For all the background on this saga, see: https://bugzilla.redhat.com/227011 * src/util/iptables.c: remove lokkit support * configure.in: remove --enable-lokkit * libvirt.spec.in: remove the dirs used only for saving rules for lokkit * src/Makefile.am: ditto * src/libvirt_private.syms, src/network/bridge_driver.c, src/util/iptables.h: remove references to iptablesSaveRules --- configure.in | 21 ---- libvirt.spec.in | 3 - src/Makefile.am | 4 - src/libvirt_private.syms | 1 - src/network/bridge_driver.c | 3 - src/util/iptables.c | 218 ------------------------------------------- src/util/iptables.h | 1 - 7 files changed, 0 insertions(+), 251 deletions(-) diff --git a/configure.in b/configure.in index 8d21207..fe9834d 100644 --- a/configure.in +++ b/configure.in @@ -269,27 +269,6 @@ if test x"$with_rhel5_api" = x"yes"; then AC_DEFINE([WITH_RHEL5_API], [1], [whether building for the RHEL-5 API]) fi -dnl -dnl ensure that Fedora's system-config-firewall knows -dnl about libvirt's iptables rules -dnl -AC_ARG_ENABLE([iptables-lokkit], - [AC_HELP_STRING([--enable-iptables-lokkit=no/yes/check], - [enable registering libvirt's iptables rules with Fedora's lokkit])], - [],[enable_iptables_lokkit=check]) -if test x"$enable_iptables_lokkit" != x"no"; then - AC_PATH_PROG([LOKKIT_PATH],[lokkit], [], [/usr/sbin:$PATH]) -fi - -if test x"$enable_iptables_lokkit" = x"yes" -a x"$LOKKIT_PATH" = x; then - AC_MSG_ERROR([Cannot find lokkit and --enable-iptables-lokkit specified]) -fi - -if test x"$LOKKIT_PATH" != x; then - AC_DEFINE([ENABLE_IPTABLES_LOKKIT], [], [whether support for Fedora's lokkit is enabled]) - AC_DEFINE_UNQUOTED([LOKKIT_PATH], "$LOKKIT_PATH", [path to lokkit binary]) -fi - AC_PATH_PROG([IPTABLES_PATH], [iptables], /sbin/iptables, [/usr/sbin:$PATH]) AC_DEFINE_UNQUOTED([IPTABLES_PATH], "$IPTABLES_PATH", [path to iptables binary]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 408ad05..dd067ad 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -710,9 +710,6 @@ fi %if %{with_network} %dir %{_localstatedir}/run/libvirt/network/ %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/network/ -%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/iptables/ -%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/iptables/filter/ -%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/iptables/nat/ %endif %if %{with_qemu} diff --git a/src/Makefile.am b/src/Makefile.am index e5d8933..b639915 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -883,8 +883,6 @@ if WITH_UML $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/uml" endif if WITH_NETWORK - $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/filter" - $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/nat" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/network" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/network" $(MKDIR_P) "$(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart" @@ -921,8 +919,6 @@ if WITH_NETWORK rm -f $(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/default.xml rmdir "$(DESTDIR)$(sysconfdir)/libvirt/qemu/networks/autostart" || : rmdir "$(DESTDIR)$(sysconfdir)/libvirt/qemu/networks" || : - rmdir "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/filter" ||: - rmdir "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/nat" ||: rmdir "$(DESTDIR)$(localstatedir)/lib/libvirt/network" ||: rmdir "$(DESTDIR)$(localstatedir)/run/libvirt/network" ||: endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 58f99fb..8d64b15 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -247,7 +247,6 @@ iptablesRemoveForwardRejectIn; iptablesRemoveForwardRejectOut; iptablesRemoveTcpInput; iptablesRemoveUdpInput; -iptablesSaveRules; # libvirt_internal.h diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d5cab71..abee78c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -752,8 +752,6 @@ networkAddIptablesRules(virConnectPtr conn, !networkAddRoutingIptablesRules(conn, driver, network)) goto err8; - iptablesSaveRules(driver->iptables); - return 1; err8: @@ -807,7 +805,6 @@ networkRemoveIptablesRules(struct network_driver *driver, iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53); iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67); iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67); - iptablesSaveRules(driver->iptables); } static void diff --git a/src/util/iptables.c b/src/util/iptables.c index 36d65e4..8ac7786 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -66,14 +66,6 @@ typedef struct int nrules; iptRule *rules; - -#ifdef ENABLE_IPTABLES_LOKKIT - - char dir[PATH_MAX]; - char path[PATH_MAX]; - -#endif /* ENABLE_IPTABLES_LOKKIT */ - } iptRules; struct _iptablesContext @@ -83,186 +75,6 @@ struct _iptablesContext iptRules *nat_postrouting; }; -#ifdef ENABLE_IPTABLES_LOKKIT -static void -notifyRulesUpdated(const char *table, - const char *path) -{ - char arg[PATH_MAX]; - const char *argv[4]; - - snprintf(arg, sizeof(arg), "--custom-rules=ipv4:%s:%s", table, path); - - argv[0] = (char *) LOKKIT_PATH; - argv[1] = (char *) "--nostart"; - argv[2] = arg; - argv[3] = NULL; - - char ebuf[1024]; - if (virRun(NULL, argv, NULL) < 0) - VIR_WARN(_("Failed to run '%s %s': %s"), - LOKKIT_PATH, arg, virStrerror(errno, ebuf, sizeof ebuf)); -} - -static int -stripLine(char *str, int len, const char *line) -{ - char *s, *p; - int changed; - - changed = 0; - s = str; - - while ((p = strchr(s, '\n'))) { - if (p == s || STRNEQLEN(s, line, p - s)) { - s = ++p; - continue; - } - - ++p; - memmove(s, p, len - (p - str) + 1); - len -= p - s; - changed = 1; - } - - if (STREQ(s, line)) { - *s = '\0'; - changed = 1; - } - - return changed; -} - -static void -notifyRulesRemoved(const char *table, - const char *path) -{ -/* 10 MB limit on config file size as a sanity check */ -#define MAX_FILE_LEN (1024*1024*10) - - char arg[PATH_MAX]; - char *content; - int len; - FILE *f = NULL; - - len = virFileReadAll(SYSCONF_DIR "/sysconfig/system-config-firewall", - MAX_FILE_LEN, &content); - if (len < 0) { - VIR_WARN("%s", _("Failed to read " SYSCONF_DIR - "/sysconfig/system-config-firewall")); - return; - } - - snprintf(arg, sizeof(arg), "--custom-rules=ipv4:%s:%s", table, path); - - if (!stripLine(content, len, arg)) { - VIR_FREE(content); - return; - } - - if (!(f = fopen(SYSCONF_DIR "/sysconfig/system-config-firewall", "w"))) - goto write_error; - - if (fputs(content, f) == EOF) - goto write_error; - - if (fclose(f) == EOF) { - f = NULL; - goto write_error; - } - - VIR_FREE(content); - - return; - - write_error:; - char ebuf[1024]; - VIR_WARN(_("Failed to write to " SYSCONF_DIR - "/sysconfig/system-config-firewall : %s"), - virStrerror(errno, ebuf, sizeof ebuf)); - if (f) - fclose(f); - VIR_FREE(content); - -#undef MAX_FILE_LEN -} - -static int -writeRules(const char *path, - const iptRule *rules, - int nrules) -{ - char tmp[PATH_MAX]; - FILE *f; - int istmp; - int i; - - if (nrules == 0 && unlink(path) == 0) - return 0; - - if (snprintf(tmp, PATH_MAX, "%s.new", path) >= PATH_MAX) - return EINVAL; - - istmp = 1; - - if (!(f = fopen(tmp, "w"))) { - istmp = 0; - if (!(f = fopen(path, "w"))) - return errno; - } - - for (i = 0; i < nrules; i++) { - if (fputs(rules[i].rule, f) == EOF || - fputc('\n', f) == EOF) { - fclose(f); - if (istmp) - unlink(tmp); - return errno; - } - } - - fclose(f); - - if (istmp && rename(tmp, path) < 0) { - unlink(tmp); - return errno; - } - - if (istmp) - unlink(tmp); - - return 0; -} -#endif /* ENABLE_IPTABLES_LOKKIT */ - -static void -iptRulesSave(iptRules *rules) -{ -#ifdef ENABLE_IPTABLES_LOKKIT - int err; - - char ebuf[1024]; - if ((err = virFileMakePath(rules->dir))) { - VIR_WARN(_("Failed to create directory %s : %s"), - rules->dir, virStrerror(err, ebuf, sizeof ebuf)); - return; - } - - if ((err = writeRules(rules->path, rules->rules, rules->nrules))) { - VIR_WARN(_("Failed to saves iptables rules to %s : %s"), - rules->path, virStrerror(err, ebuf, sizeof ebuf)); - return; - } - - if (rules->nrules > 0) - notifyRulesUpdated(rules->table, rules->path); - else - notifyRulesRemoved(rules->table, rules->path); -#else - (void) rules; -#endif /* ENABLE_IPTABLES_LOKKIT */ -} - static void iptRuleFree(iptRule *rule) { @@ -340,11 +152,6 @@ iptRulesFree(iptRules *rules) rules->nrules = 0; } -#ifdef ENABLE_IPTABLES_LOKKIT - rules->dir[0] = '\0'; - rules->path[0] = '\0'; -#endif /* ENABLE_IPTABLES_LOKKIT */ - VIR_FREE(rules); } @@ -366,15 +173,6 @@ iptRulesNew(const char *table, rules->rules = NULL; rules->nrules = 0; -#ifdef ENABLE_IPTABLES_LOKKIT - if (virFileBuildPath(LOCAL_STATE_DIR "/lib/libvirt/iptables", table, NULL, - rules->dir, sizeof(rules->dir)) < 0) - goto error; - - if (virFileBuildPath(rules->dir, chain, ".chain", rules->path, sizeof(rules->path)) < 0) - goto error; -#endif /* ENABLE_IPTABLES_LOKKIT */ - return rules; error: @@ -520,22 +318,6 @@ iptablesContextFree(iptablesContext *ctx) VIR_FREE(ctx); } -/** - * iptablesSaveRules: - * @ctx: pointer to the IP table context - * - * Saves all the IP table rules associated with a context - * to disk so that if iptables is restarted, the rules - * will automatically be reload. - */ -void -iptablesSaveRules(iptablesContext *ctx) -{ - iptRulesSave(ctx->input_filter); - iptRulesSave(ctx->forward_filter); - iptRulesSave(ctx->nat_postrouting); -} - static void iptRulesReload(iptRules *rules) { diff --git a/src/util/iptables.h b/src/util/iptables.h index fbe9b5d..826f4f8 100644 --- a/src/util/iptables.h +++ b/src/util/iptables.h @@ -27,7 +27,6 @@ typedef struct _iptablesContext iptablesContext; iptablesContext *iptablesContextNew (void); void iptablesContextFree (iptablesContext *ctx); -void iptablesSaveRules (iptablesContext *ctx); void iptablesReloadRules (iptablesContext *ctx); int iptablesAddTcpInput (iptablesContext *ctx, -- 1.6.5.2

Daniel P. Berrange

1:04 p.m.

New subject: [libvirt] [PATCH 3/5] remove all traces of lokkit support

On Thu, Dec 10, 2009 at 11:27:53AM +0000, Mark McLoughlin wrote:

...

Long ago we tried to use Fedora's lokkit utility in order to register our iptables rules so that 'service iptables restart' would automatically load our rules.

There was one fatal flaw - if the user had configured iptables without lokkit, then we would clobber that configuration by running lokkit.

We quickly disabled lokkit support, but never removed it. Let's do that now.

The 'my virtual network stops working when I restart iptables' still remains. For all the background on this saga, see:

https://bugzilla.redhat.com/227011

* src/util/iptables.c: remove lokkit support

* configure.in: remove --enable-lokkit

* libvirt.spec.in: remove the dirs used only for saving rules for lokkit

* src/Makefile.am: ditto

* src/libvirt_private.syms, src/network/bridge_driver.c, src/util/iptables.h: remove references to iptablesSaveRules --- configure.in | 21 ---- libvirt.spec.in | 3 - src/Makefile.am | 4 - src/libvirt_private.syms | 1 - src/network/bridge_driver.c | 3 - src/util/iptables.c | 218 ------------------------------------------- src/util/iptables.h | 1 - 7 files changed, 0 insertions(+), 251 deletions(-)

ACK, I meant to send this myself in fact. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Mark McLoughlin

12:27 p.m.

New subject: [libvirt] [PATCH 4/5] remove iptablesReloadRules() and related code

We don't use this method of reloading rules anymore, so we can just kill the code. This simplifies things a lot because we no longer need to keep a table of the rules we've added. * src/util/iptables.c: kill iptablesReloadRules() --- src/libvirt_private.syms | 1 - src/util/iptables.c | 155 +--------------------------------------------- src/util/iptables.h | 2 - 3 files changed, 3 insertions(+), 155 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d64b15..e5ba365 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -237,7 +237,6 @@ iptablesAddTcpInput; iptablesAddUdpInput; iptablesContextFree; iptablesContextNew; -iptablesReloadRules; iptablesRemoveForwardAllowCross; iptablesRemoveForwardAllowIn; iptablesRemoveForwardAllowOut; diff --git a/src/util/iptables.c b/src/util/iptables.c index 8ac7786..3c02ea6 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -54,18 +54,8 @@ enum { typedef struct { - char *rule; - const char **argv; - int command_idx; -} iptRule; - -typedef struct -{ char *table; char *chain; - - int nrules; - iptRule *rules; } iptRules; struct _iptablesContext @@ -76,82 +66,10 @@ struct _iptablesContext }; static void -iptRuleFree(iptRule *rule) -{ - VIR_FREE(rule->rule); - - if (rule->argv) { - int i = 0; - while (rule->argv[i]) - VIR_FREE(rule->argv[i++]); - VIR_FREE(rule->argv); - } -} - -static int -iptRulesAppend(iptRules *rules, - char *rule, - const char **argv, - int command_idx) -{ - if (VIR_REALLOC_N(rules->rules, rules->nrules+1) < 0) { - int i = 0; - while (argv[i]) - VIR_FREE(argv[i++]); - VIR_FREE(argv); - return ENOMEM; - } - - rules->rules[rules->nrules].rule = rule; - rules->rules[rules->nrules].argv = argv; - rules->rules[rules->nrules].command_idx = command_idx; - - rules->nrules++; - - return 0; -} - -static int -iptRulesRemove(iptRules *rules, - char *rule) -{ - int i; - - for (i = 0; i < rules->nrules; i++) - if (STREQ(rules->rules[i].rule, rule)) - break; - - if (i >= rules->nrules) - return EINVAL; - - iptRuleFree(&rules->rules[i]); - - memmove(&rules->rules[i], - &rules->rules[i+1], - (rules->nrules - i - 1) * sizeof (iptRule)); - - rules->nrules--; - - return 0; -} - -static void iptRulesFree(iptRules *rules) { - int i; - VIR_FREE(rules->table); VIR_FREE(rules->chain); - - if (rules->rules) { - for (i = 0; i < rules->nrules; i++) - iptRuleFree(&rules->rules[i]); - - VIR_FREE(rules->rules); - - rules->nrules = 0; - } - VIR_FREE(rules); } @@ -170,9 +88,6 @@ iptRulesNew(const char *table, if (!(rules->chain = strdup(chain))) goto error; - rules->rules = NULL; - rules->nrules = 0; - return rules; error: @@ -186,9 +101,8 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) va_list args; int retval = ENOMEM; const char **argv; - char *rule = NULL; const char *s; - int n, command_idx; + int n; n = 1 + /* /sbin/iptables */ 2 + /* --table foo */ @@ -215,9 +129,7 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) if (!(argv[n++] = strdup(rules->table))) goto error; - command_idx = n; - - if (!(argv[n++] = strdup("--insert"))) + if (!(argv[n++] = strdup(action == ADD ? "--insert" : "--delete"))) goto error; if (!(argv[n++] = strdup(rules->chain))) @@ -234,31 +146,14 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) va_end(args); - if (!(rule = virArgvToString(&argv[command_idx]))) - goto error; - - if (action == REMOVE) { - VIR_FREE(argv[command_idx]); - if (!(argv[command_idx] = strdup("--delete"))) - goto error; - } - if (virRun(NULL, argv, NULL) < 0) { retval = errno; goto error; } - if (action == ADD) { - retval = iptRulesAppend(rules, rule, argv, command_idx); - rule = NULL; - argv = NULL; - } else { - retval = iptRulesRemove(rules, rule); - } + retval = 0; error: - VIR_FREE(rule); - if (argv) { n = 0; while (argv[n]) @@ -318,50 +213,6 @@ iptablesContextFree(iptablesContext *ctx) VIR_FREE(ctx); } -static void -iptRulesReload(iptRules *rules) -{ - int i; - char ebuf[1024]; - - for (i = 0; i < rules->nrules; i++) { - iptRule *rule = &rules->rules[i]; - const char *orig; - - orig = rule->argv[rule->command_idx]; - rule->argv[rule->command_idx] = (char *) "--delete"; - - if (virRun(NULL, rule->argv, NULL) < 0) - VIR_WARN(_("Failed to remove iptables rule '%s'" - " from chain '%s' in table '%s': %s"), - rule->rule, rules->chain, rules->table, - virStrerror(errno, ebuf, sizeof ebuf)); - - rule->argv[rule->command_idx] = orig; - } - - for (i = 0; i < rules->nrules; i++) - if (virRun(NULL, rules->rules[i].argv, NULL) < 0) - VIR_WARN(_("Failed to add iptables rule '%s'" - " to chain '%s' in table '%s': %s"), - rules->rules[i].rule, rules->chain, rules->table, - virStrerror(errno, ebuf, sizeof ebuf)); -} - -/** - * iptablesReloadRules: - * @ctx: pointer to the IP table context - * - * Reloads all the IP table rules associated to a context - */ -void -iptablesReloadRules(iptablesContext *ctx) -{ - iptRulesReload(ctx->input_filter); - iptRulesReload(ctx->forward_filter); - iptRulesReload(ctx->nat_postrouting); -} - static int iptablesInput(iptablesContext *ctx, const char *iface, diff --git a/src/util/iptables.h b/src/util/iptables.h index 826f4f8..68d9e0d 100644 --- a/src/util/iptables.h +++ b/src/util/iptables.h @@ -27,8 +27,6 @@ typedef struct _iptablesContext iptablesContext; iptablesContext *iptablesContextNew (void); void iptablesContextFree (iptablesContext *ctx); -void iptablesReloadRules (iptablesContext *ctx); - int iptablesAddTcpInput (iptablesContext *ctx, const char *iface, int port); -- 1.6.5.2

Daniel P. Berrange

1:05 p.m.

New subject: [libvirt] [PATCH 4/5] remove iptablesReloadRules() and related code

On Thu, Dec 10, 2009 at 11:27:54AM +0000, Mark McLoughlin wrote:

...

We don't use this method of reloading rules anymore, so we can just kill the code.

This simplifies things a lot because we no longer need to keep a table of the rules we've added.

* src/util/iptables.c: kill iptablesReloadRules() --- src/libvirt_private.syms | 1 - src/util/iptables.c | 155 +--------------------------------------------- src/util/iptables.h | 2 - 3 files changed, 3 insertions(+), 155 deletions(-)

Mark McLoughlin

12:27 p.m.

New subject: [libvirt] [PATCH 5/5] remove now unneeded iptablesContext

iptablesContext no longer contains any state, so we can drop it * src/util/iptables.c, src/util/iptables.h: drop iptablesContext * src/network/bridge_driver.c: update callers * src/libvirt_private.syms: drop context new/free functions --- src/libvirt_private.syms | 2 - src/network/bridge_driver.c | 132 ++++++++----------- src/util/iptables.c | 307 ++++++++++++------------------------------- src/util/iptables.h | 59 +++------ 4 files changed, 153 insertions(+), 347 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e5ba365..d78142e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -235,8 +235,6 @@ iptablesAddForwardRejectIn; iptablesAddForwardRejectOut; iptablesAddTcpInput; iptablesAddUdpInput; -iptablesContextFree; -iptablesContextNew; iptablesRemoveForwardAllowCross; iptablesRemoveForwardAllowIn; iptablesRemoveForwardAllowOut; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index abee78c..28340a1 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -69,7 +69,6 @@ struct network_driver { virNetworkObjList networks; - iptablesContext *iptables; brControl *brctl; char *networkConfigDir; char *networkAutostartDir; @@ -247,11 +246,6 @@ networkStartup(int privileged) { goto error; } - if (!(driverState->iptables = iptablesContextNew())) { - goto out_of_memory; - } - - if (virNetworkLoadAllConfigs(NULL, &driverState->networks, driverState->networkConfigDir, @@ -349,8 +343,6 @@ networkShutdown(void) { if (driverState->brctl) brShutdown(driverState->brctl); - if (driverState->iptables) - iptablesContextFree(driverState->iptables); networkDriverUnlock(driverState); virMutexDestroy(&driverState->lock); @@ -590,13 +582,11 @@ cleanup: } static int -networkAddMasqueradingIptablesRules(virConnectPtr conn, - struct network_driver *driver, - virNetworkObjPtr network) { +networkAddMasqueradingIptablesRules(virConnectPtr conn, virNetworkObjPtr network) +{ int err; /* allow forwarding packets from the bridge interface */ - if ((err = iptablesAddForwardAllowOut(driver->iptables, - network->def->network, + if ((err = iptablesAddForwardAllowOut(network->def->network, network->def->bridge, network->def->forwardDev))) { virReportSystemError(conn, err, @@ -606,10 +596,9 @@ networkAddMasqueradingIptablesRules(virConnectPtr conn, } /* allow forwarding packets to the bridge interface if they are part of an existing connection */ - if ((err = iptablesAddForwardAllowRelatedIn(driver->iptables, - network->def->network, - network->def->bridge, - network->def->forwardDev))) { + if ((err = iptablesAddForwardAllowRelatedIn(network->def->network, + network->def->bridge, + network->def->forwardDev))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow forwarding to '%s'"), network->def->bridge); @@ -617,8 +606,7 @@ networkAddMasqueradingIptablesRules(virConnectPtr conn, } /* enable masquerading */ - if ((err = iptablesAddForwardMasquerade(driver->iptables, - network->def->network, + if ((err = iptablesAddForwardMasquerade(network->def->network, network->def->forwardDev))) { virReportSystemError(conn, err, _("failed to add iptables rule to enable masquerading to '%s'\n"), @@ -629,13 +617,11 @@ networkAddMasqueradingIptablesRules(virConnectPtr conn, return 1; masqerr3: - iptablesRemoveForwardAllowRelatedIn(driver->iptables, - network->def->network, - network->def->bridge, - network->def->forwardDev); + iptablesRemoveForwardAllowRelatedIn(network->def->network, + network->def->bridge, + network->def->forwardDev); masqerr2: - iptablesRemoveForwardAllowOut(driver->iptables, - network->def->network, + iptablesRemoveForwardAllowOut(network->def->network, network->def->bridge, network->def->forwardDev); masqerr1: @@ -643,13 +629,11 @@ networkAddMasqueradingIptablesRules(virConnectPtr conn, } static int -networkAddRoutingIptablesRules(virConnectPtr conn, - struct network_driver *driver, - virNetworkObjPtr network) { +networkAddRoutingIptablesRules(virConnectPtr conn, virNetworkObjPtr network) +{ int err; /* allow routing packets from the bridge interface */ - if ((err = iptablesAddForwardAllowOut(driver->iptables, - network->def->network, + if ((err = iptablesAddForwardAllowOut(network->def->network, network->def->bridge, network->def->forwardDev))) { virReportSystemError(conn, err, @@ -659,8 +643,7 @@ networkAddRoutingIptablesRules(virConnectPtr conn, } /* allow routing packets to the bridge interface */ - if ((err = iptablesAddForwardAllowIn(driver->iptables, - network->def->network, + if ((err = iptablesAddForwardAllowIn(network->def->network, network->def->bridge, network->def->forwardDev))) { virReportSystemError(conn, err, @@ -673,8 +656,7 @@ networkAddRoutingIptablesRules(virConnectPtr conn, routeerr2: - iptablesRemoveForwardAllowOut(driver->iptables, - network->def->network, + iptablesRemoveForwardAllowOut(network->def->network, network->def->bridge, network->def->forwardDev); routeerr1: @@ -682,20 +664,19 @@ networkAddRoutingIptablesRules(virConnectPtr conn, } static int -networkAddIptablesRules(virConnectPtr conn, - struct network_driver *driver, - virNetworkObjPtr network) { +networkAddIptablesRules(virConnectPtr conn, virNetworkObjPtr network) +{ int err; /* allow DHCP requests through to dnsmasq */ - if ((err = iptablesAddTcpInput(driver->iptables, network->def->bridge, 67))) { + if ((err = iptablesAddTcpInput(network->def->bridge, 67))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow DHCP requests from '%s'"), network->def->bridge); goto err1; } - if ((err = iptablesAddUdpInput(driver->iptables, network->def->bridge, 67))) { + if ((err = iptablesAddUdpInput(network->def->bridge, 67))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow DHCP requests from '%s'"), network->def->bridge); @@ -703,14 +684,14 @@ networkAddIptablesRules(virConnectPtr conn, } /* allow DNS requests through to dnsmasq */ - if ((err = iptablesAddTcpInput(driver->iptables, network->def->bridge, 53))) { + if ((err = iptablesAddTcpInput(network->def->bridge, 53))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow DNS requests from '%s'"), network->def->bridge); goto err3; } - if ((err = iptablesAddUdpInput(driver->iptables, network->def->bridge, 53))) { + if ((err = iptablesAddUdpInput(network->def->bridge, 53))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow DNS requests from '%s'"), network->def->bridge); @@ -720,14 +701,14 @@ networkAddIptablesRules(virConnectPtr conn, /* Catch all rules to block forwarding to/from bridges */ - if ((err = iptablesAddForwardRejectOut(driver->iptables, network->def->bridge))) { + if ((err = iptablesAddForwardRejectOut(network->def->bridge))) { virReportSystemError(conn, err, _("failed to add iptables rule to block outbound traffic from '%s'"), network->def->bridge); goto err5; } - if ((err = iptablesAddForwardRejectIn(driver->iptables, network->def->bridge))) { + if ((err = iptablesAddForwardRejectIn(network->def->bridge))) { virReportSystemError(conn, err, _("failed to add iptables rule to block inbound traffic to '%s'"), network->def->bridge); @@ -735,7 +716,7 @@ networkAddIptablesRules(virConnectPtr conn, } /* Allow traffic between guests on the same bridge */ - if ((err = iptablesAddForwardAllowCross(driver->iptables, network->def->bridge))) { + if ((err = iptablesAddForwardAllowCross(network->def->bridge))) { virReportSystemError(conn, err, _("failed to add iptables rule to allow cross bridge traffic on '%s'"), network->def->bridge); @@ -745,66 +726,59 @@ networkAddIptablesRules(virConnectPtr conn, /* If masquerading is enabled, set up the rules*/ if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT && - !networkAddMasqueradingIptablesRules(conn, driver, network)) + !networkAddMasqueradingIptablesRules(conn, network)) goto err8; /* else if routing is enabled, set up the rules*/ else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE && - !networkAddRoutingIptablesRules(conn, driver, network)) + !networkAddRoutingIptablesRules(conn, network)) goto err8; return 1; err8: - iptablesRemoveForwardAllowCross(driver->iptables, - network->def->bridge); + iptablesRemoveForwardAllowCross(network->def->bridge); err7: - iptablesRemoveForwardRejectIn(driver->iptables, - network->def->bridge); + iptablesRemoveForwardRejectIn(network->def->bridge); err6: - iptablesRemoveForwardRejectOut(driver->iptables, - network->def->bridge); + iptablesRemoveForwardRejectOut(network->def->bridge); err5: - iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53); + iptablesRemoveUdpInput(network->def->bridge, 53); err4: - iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53); + iptablesRemoveTcpInput(network->def->bridge, 53); err3: - iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67); + iptablesRemoveUdpInput(network->def->bridge, 67); err2: - iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67); + iptablesRemoveTcpInput(network->def->bridge, 67); err1: return 0; } static void -networkRemoveIptablesRules(struct network_driver *driver, - virNetworkObjPtr network) { +networkRemoveIptablesRules(virNetworkObjPtr network) +{ if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE) { if (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) { - iptablesRemoveForwardMasquerade(driver->iptables, - network->def->network, - network->def->forwardDev); - iptablesRemoveForwardAllowRelatedIn(driver->iptables, - network->def->network, + iptablesRemoveForwardMasquerade(network->def->network, + network->def->forwardDev); + iptablesRemoveForwardAllowRelatedIn(network->def->network, network->def->bridge, network->def->forwardDev); } else if (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE) - iptablesRemoveForwardAllowIn(driver->iptables, - network->def->network, + iptablesRemoveForwardAllowIn(network->def->network, network->def->bridge, network->def->forwardDev); - iptablesRemoveForwardAllowOut(driver->iptables, - network->def->network, + iptablesRemoveForwardAllowOut(network->def->network, network->def->bridge, network->def->forwardDev); } - iptablesRemoveForwardAllowCross(driver->iptables, network->def->bridge); - iptablesRemoveForwardRejectIn(driver->iptables, network->def->bridge); - iptablesRemoveForwardRejectOut(driver->iptables, network->def->bridge); - iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 53); - iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 53); - iptablesRemoveUdpInput(driver->iptables, network->def->bridge, 67); - iptablesRemoveTcpInput(driver->iptables, network->def->bridge, 67); + iptablesRemoveForwardAllowCross(network->def->bridge); + iptablesRemoveForwardRejectIn(network->def->bridge); + iptablesRemoveForwardRejectOut(network->def->bridge); + iptablesRemoveUdpInput(network->def->bridge, 53); + iptablesRemoveTcpInput(network->def->bridge, 53); + iptablesRemoveUdpInput(network->def->bridge, 67); + iptablesRemoveTcpInput(network->def->bridge, 67); } static void @@ -818,8 +792,8 @@ networkReloadIptablesRules(struct network_driver *driver) virNetworkObjLock(driver->networks.objs[i]); if (virNetworkObjIsActive(driver->networks.objs[i])) { - networkRemoveIptablesRules(driver, driver->networks.objs[i]); - if (!networkAddIptablesRules(NULL, driver, driver->networks.objs[i])) { + networkRemoveIptablesRules(driver->networks.objs[i]); + if (!networkAddIptablesRules(NULL, driver->networks.objs[i])) { /* failed to add but already logged */ } } @@ -940,7 +914,7 @@ static int networkStartNetworkDaemon(virConnectPtr conn, goto err_delbr; } - if (!networkAddIptablesRules(conn, driver, network)) + if (!networkAddIptablesRules(conn, network)) goto err_delbr1; if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE && @@ -972,7 +946,7 @@ static int networkStartNetworkDaemon(virConnectPtr conn, } err_delbr2: - networkRemoveIptablesRules(driver, network); + networkRemoveIptablesRules(network); err_delbr1: if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { @@ -1013,7 +987,7 @@ static int networkShutdownNetworkDaemon(virConnectPtr conn, if (network->dnsmasqPid > 0) kill(network->dnsmasqPid, SIGTERM); - networkRemoveIptablesRules(driver, network); + networkRemoveIptablesRules(network); char ebuf[1024]; if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { diff --git a/src/util/iptables.c b/src/util/iptables.c index 3c02ea6..de75a24 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -52,51 +52,9 @@ enum { REMOVE }; -typedef struct -{ - char *table; - char *chain; -} iptRules; - -struct _iptablesContext -{ - iptRules *input_filter; - iptRules *forward_filter; - iptRules *nat_postrouting; -}; - -static void -iptRulesFree(iptRules *rules) -{ - VIR_FREE(rules->table); - VIR_FREE(rules->chain); - VIR_FREE(rules); -} - -static iptRules * -iptRulesNew(const char *table, - const char *chain) -{ - iptRules *rules; - - if (VIR_ALLOC(rules) < 0) - return NULL; - - if (!(rules->table = strdup(table))) - goto error; - - if (!(rules->chain = strdup(chain))) - goto error; - - return rules; - - error: - iptRulesFree(rules); - return NULL; -} - static int ATTRIBUTE_SENTINEL -iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) +iptablesAddRemoveRule(const char *table, const char *chain, + int action, const char *arg, ...) { va_list args; int retval = ENOMEM; @@ -126,13 +84,13 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) if (!(argv[n++] = strdup("--table"))) goto error; - if (!(argv[n++] = strdup(rules->table))) + if (!(argv[n++] = strdup(table))) goto error; if (!(argv[n++] = strdup(action == ADD ? "--insert" : "--delete"))) goto error; - if (!(argv[n++] = strdup(rules->chain))) + if (!(argv[n++] = strdup(chain))) goto error; if (!(argv[n++] = strdup(arg))) @@ -164,58 +122,8 @@ iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) return retval; } -/** - * iptablesContextNew: - * - * Create a new IPtable context - * - * Returns a pointer to the new structure or NULL in case of error - */ -iptablesContext * -iptablesContextNew(void) -{ - iptablesContext *ctx; - - if (VIR_ALLOC(ctx) < 0) - return NULL; - - if (!(ctx->input_filter = iptRulesNew("filter", "INPUT"))) - goto error; - - if (!(ctx->forward_filter = iptRulesNew("filter", "FORWARD"))) - goto error; - - if (!(ctx->nat_postrouting = iptRulesNew("nat", "POSTROUTING"))) - goto error; - - return ctx; - - error: - iptablesContextFree(ctx); - return NULL; -} - -/** - * iptablesContextFree: - * @ctx: pointer to the IP table context - * - * Free the resources associated with an IP table context - */ -void -iptablesContextFree(iptablesContext *ctx) -{ - if (ctx->input_filter) - iptRulesFree(ctx->input_filter); - if (ctx->forward_filter) - iptRulesFree(ctx->forward_filter); - if (ctx->nat_postrouting) - iptRulesFree(ctx->nat_postrouting); - VIR_FREE(ctx); -} - static int -iptablesInput(iptablesContext *ctx, - const char *iface, +iptablesInput(const char *iface, int port, int action, int tcp) @@ -225,7 +133,7 @@ iptablesInput(iptablesContext *ctx, snprintf(portstr, sizeof(portstr), "%d", port); portstr[sizeof(portstr) - 1] = '\0'; - return iptablesAddRemoveRule(ctx->input_filter, + return iptablesAddRemoveRule("filter", "INPUT", action, "--in-interface", iface, "--protocol", tcp ? "tcp" : "udp", @@ -236,7 +144,6 @@ iptablesInput(iptablesContext *ctx, /** * iptablesAddTcpInput: - * @ctx: pointer to the IP table context * @iface: the interface name * @port: the TCP port to add * @@ -247,16 +154,13 @@ iptablesInput(iptablesContext *ctx, */ int -iptablesAddTcpInput(iptablesContext *ctx, - const char *iface, - int port) +iptablesAddTcpInput(const char *iface, int port) { - return iptablesInput(ctx, iface, port, ADD, 1); + return iptablesInput(iface, port, ADD, 1); } /** * iptablesRemoveTcpInput: - * @ctx: pointer to the IP table context * @iface: the interface name * @port: the TCP port to remove * @@ -266,16 +170,13 @@ iptablesAddTcpInput(iptablesContext *ctx, * Returns 0 in case of success or an error code in case of error */ int -iptablesRemoveTcpInput(iptablesContext *ctx, - const char *iface, - int port) +iptablesRemoveTcpInput(const char *iface, int port) { - return iptablesInput(ctx, iface, port, REMOVE, 1); + return iptablesInput(iface, port, REMOVE, 1); } /** * iptablesAddUdpInput: - * @ctx: pointer to the IP table context * @iface: the interface name * @port: the UDP port to add * @@ -286,16 +187,13 @@ iptablesRemoveTcpInput(iptablesContext *ctx, */ int -iptablesAddUdpInput(iptablesContext *ctx, - const char *iface, - int port) +iptablesAddUdpInput(const char *iface, int port) { - return iptablesInput(ctx, iface, port, ADD, 0); + return iptablesInput(iface, port, ADD, 0); } /** * iptablesRemoveUdpInput: - * @ctx: pointer to the IP table context * @iface: the interface name * @port: the UDP port to remove * @@ -305,11 +203,9 @@ iptablesAddUdpInput(iptablesContext *ctx, * Returns 0 in case of success or an error code in case of error */ int -iptablesRemoveUdpInput(iptablesContext *ctx, - const char *iface, - int port) +iptablesRemoveUdpInput(const char *iface, int port) { - return iptablesInput(ctx, iface, port, REMOVE, 0); + return iptablesInput(iface, port, REMOVE, 0); } @@ -317,14 +213,13 @@ iptablesRemoveUdpInput(iptablesContext *ctx, * to proceed to WAN */ static int -iptablesForwardAllowOut(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev, - int action) +iptablesForwardAllowOut(const char *network, + const char *iface, + const char *physdev, + int action) { if (physdev && physdev[0]) { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--source", network, "--in-interface", iface, @@ -332,7 +227,7 @@ iptablesForwardAllowOut(iptablesContext *ctx, "--jump", "ACCEPT", NULL); } else { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--source", network, "--in-interface", iface, @@ -343,7 +238,6 @@ iptablesForwardAllowOut(iptablesContext *ctx, /** * iptablesAddForwardAllowOut: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the source interface name * @physdev: the physical output device @@ -355,17 +249,15 @@ iptablesForwardAllowOut(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardAllowOut(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev) +iptablesAddForwardAllowOut(const char *network, + const char *iface, + const char *physdev) { - return iptablesForwardAllowOut(ctx, network, iface, physdev, ADD); + return iptablesForwardAllowOut(network, iface, physdev, ADD); } /** * iptablesRemoveForwardAllowOut: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the source interface name * @physdev: the physical output device @@ -377,12 +269,11 @@ iptablesAddForwardAllowOut(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardAllowOut(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev) +iptablesRemoveForwardAllowOut(const char *network, + const char *iface, + const char *physdev) { - return iptablesForwardAllowOut(ctx, network, iface, physdev, REMOVE); + return iptablesForwardAllowOut(network, iface, physdev, REMOVE); } @@ -390,14 +281,13 @@ iptablesRemoveForwardAllowOut(iptablesContext *ctx, * and associated with an existing connection */ static int -iptablesForwardAllowRelatedIn(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev, - int action) +iptablesForwardAllowRelatedIn(const char *network, + const char *iface, + const char *physdev, + int action) { if (physdev && physdev[0]) { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--destination", network, "--in-interface", physdev, @@ -407,7 +297,7 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx, "--jump", "ACCEPT", NULL); } else { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--destination", network, "--out-interface", iface, @@ -420,7 +310,6 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx, /** * iptablesAddForwardAllowRelatedIn: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the output interface name * @physdev: the physical input device or NULL @@ -432,17 +321,15 @@ iptablesForwardAllowRelatedIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardAllowRelatedIn(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev) +iptablesAddForwardAllowRelatedIn(const char *network, + const char *iface, + const char *physdev) { - return iptablesForwardAllowRelatedIn(ctx, network, iface, physdev, ADD); + return iptablesForwardAllowRelatedIn(network, iface, physdev, ADD); } /** * iptablesRemoveForwardAllowRelatedIn: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the output interface name * @physdev: the physical input device or NULL @@ -454,25 +341,23 @@ iptablesAddForwardAllowRelatedIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardAllowRelatedIn(iptablesContext *ctx, - const char *network, - const char *iface, - const char *physdev) +iptablesRemoveForwardAllowRelatedIn(const char *network, + const char *iface, + const char *physdev) { - return iptablesForwardAllowRelatedIn(ctx, network, iface, physdev, REMOVE); + return iptablesForwardAllowRelatedIn(network, iface, physdev, REMOVE); } /* Allow all traffic destined to the bridge, with a valid network address */ static int -iptablesForwardAllowIn(iptablesContext *ctx, - const char *network, +iptablesForwardAllowIn(const char *network, const char *iface, const char *physdev, int action) { if (physdev && physdev[0]) { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--destination", network, "--in-interface", physdev, @@ -480,7 +365,7 @@ iptablesForwardAllowIn(iptablesContext *ctx, "--jump", "ACCEPT", NULL); } else { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--destination", network, "--out-interface", iface, @@ -491,7 +376,6 @@ iptablesForwardAllowIn(iptablesContext *ctx, /** * iptablesAddForwardAllowIn: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the output interface name * @physdev: the physical input device or NULL @@ -503,17 +387,15 @@ iptablesForwardAllowIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardAllowIn(iptablesContext *ctx, - const char *network, +iptablesAddForwardAllowIn(const char *network, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(ctx, network, iface, physdev, ADD); + return iptablesForwardAllowIn(network, iface, physdev, ADD); } /** * iptablesRemoveForwardAllowIn: - * @ctx: pointer to the IP table context * @network: the source network name * @iface: the output interface name * @physdev: the physical input device or NULL @@ -525,12 +407,11 @@ iptablesAddForwardAllowIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardAllowIn(iptablesContext *ctx, - const char *network, +iptablesRemoveForwardAllowIn(const char *network, const char *iface, const char *physdev) { - return iptablesForwardAllowIn(ctx, network, iface, physdev, REMOVE); + return iptablesForwardAllowIn(network, iface, physdev, REMOVE); } @@ -538,11 +419,9 @@ iptablesRemoveForwardAllowIn(iptablesContext *ctx, * with a valid network address */ static int -iptablesForwardAllowCross(iptablesContext *ctx, - const char *iface, - int action) +iptablesForwardAllowCross(const char *iface, int action) { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--in-interface", iface, "--out-interface", iface, @@ -552,7 +431,6 @@ iptablesForwardAllowCross(iptablesContext *ctx, /** * iptablesAddForwardAllowCross: - * @ctx: pointer to the IP table context * @iface: the input/output interface name * * Add rules to the IP table context to allow traffic to cross that @@ -562,14 +440,13 @@ iptablesForwardAllowCross(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardAllowCross(iptablesContext *ctx, - const char *iface) { - return iptablesForwardAllowCross(ctx, iface, ADD); +iptablesAddForwardAllowCross( const char *iface) +{ + return iptablesForwardAllowCross(iface, ADD); } /** * iptablesRemoveForwardAllowCross: - * @ctx: pointer to the IP table context * @iface: the input/output interface name * * Remove rules to the IP table context to block traffic to cross that @@ -579,9 +456,9 @@ iptablesAddForwardAllowCross(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardAllowCross(iptablesContext *ctx, - const char *iface) { - return iptablesForwardAllowCross(ctx, iface, REMOVE); +iptablesRemoveForwardAllowCross(const char *iface) +{ + return iptablesForwardAllowCross(iface, REMOVE); } @@ -589,20 +466,17 @@ iptablesRemoveForwardAllowCross(iptablesContext *ctx, * ie the bridge is the in interface */ static int -iptablesForwardRejectOut(iptablesContext *ctx, - const char *iface, - int action) +iptablesForwardRejectOut(const char *iface, int action) { - return iptablesAddRemoveRule(ctx->forward_filter, - action, - "--in-interface", iface, - "--jump", "REJECT", - NULL); + return iptablesAddRemoveRule("filter", "FORWARD", + action, + "--in-interface", iface, + "--jump", "REJECT", + NULL); } /** * iptablesAddForwardRejectOut: - * @ctx: pointer to the IP table context * @iface: the output interface name * * Add rules to the IP table context to forbid all traffic to that @@ -611,15 +485,13 @@ iptablesForwardRejectOut(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardRejectOut(iptablesContext *ctx, - const char *iface) +iptablesAddForwardRejectOut(const char *iface) { - return iptablesForwardRejectOut(ctx, iface, ADD); + return iptablesForwardRejectOut(iface, ADD); } /** * iptablesRemoveForwardRejectOut: - * @ctx: pointer to the IP table context * @iface: the output interface name * * Remove rules from the IP table context forbidding all traffic to that @@ -628,24 +500,18 @@ iptablesAddForwardRejectOut(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardRejectOut(iptablesContext *ctx, - const char *iface) +iptablesRemoveForwardRejectOut(const char *iface) { - return iptablesForwardRejectOut(ctx, iface, REMOVE); + return iptablesForwardRejectOut(iface, REMOVE); } - - - /* Drop all traffic trying to forward to the bridge. * ie the bridge is the out interface */ static int -iptablesForwardRejectIn(iptablesContext *ctx, - const char *iface, - int action) +iptablesForwardRejectIn(const char *iface, int action) { - return iptablesAddRemoveRule(ctx->forward_filter, + return iptablesAddRemoveRule("filter", "FORWARD", action, "--out-interface", iface, "--jump", "REJECT", @@ -654,7 +520,6 @@ iptablesForwardRejectIn(iptablesContext *ctx, /** * iptablesAddForwardRejectIn: - * @ctx: pointer to the IP table context * @iface: the input interface name * * Add rules to the IP table context to forbid all traffic from that @@ -663,15 +528,13 @@ iptablesForwardRejectIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardRejectIn(iptablesContext *ctx, - const char *iface) +iptablesAddForwardRejectIn(const char *iface) { - return iptablesForwardRejectIn(ctx, iface, ADD); + return iptablesForwardRejectIn(iface, ADD); } /** * iptablesRemoveForwardRejectIn: - * @ctx: pointer to the IP table context * @iface: the input interface name * * Remove rules from the IP table context forbidding all traffic from that @@ -680,10 +543,9 @@ iptablesAddForwardRejectIn(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardRejectIn(iptablesContext *ctx, - const char *iface) +iptablesRemoveForwardRejectIn(const char *iface) { - return iptablesForwardRejectIn(ctx, iface, REMOVE); + return iptablesForwardRejectIn(iface, REMOVE); } @@ -691,13 +553,12 @@ iptablesRemoveForwardRejectIn(iptablesContext *ctx, * with the bridge */ static int -iptablesForwardMasquerade(iptablesContext *ctx, - const char *network, - const char *physdev, - int action) +iptablesForwardMasquerade(const char *network, + const char *physdev, + int action) { if (physdev && physdev[0]) { - return iptablesAddRemoveRule(ctx->nat_postrouting, + return iptablesAddRemoveRule("nat", "POSTROUTING", action, "--source", network, "!", "--destination", network, @@ -705,7 +566,7 @@ iptablesForwardMasquerade(iptablesContext *ctx, "--jump", "MASQUERADE", NULL); } else { - return iptablesAddRemoveRule(ctx->nat_postrouting, + return iptablesAddRemoveRule("nat", "POSTROUTING", action, "--source", network, "!", "--destination", network, @@ -716,7 +577,6 @@ iptablesForwardMasquerade(iptablesContext *ctx, /** * iptablesAddForwardMasquerade: - * @ctx: pointer to the IP table context * @network: the source network name * @physdev: the physical input device or NULL * @@ -727,16 +587,14 @@ iptablesForwardMasquerade(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesAddForwardMasquerade(iptablesContext *ctx, - const char *network, +iptablesAddForwardMasquerade(const char *network, const char *physdev) { - return iptablesForwardMasquerade(ctx, network, physdev, ADD); + return iptablesForwardMasquerade(network, physdev, ADD); } /** * iptablesRemoveForwardMasquerade: - * @ctx: pointer to the IP table context * @network: the source network name * @physdev: the physical input device or NULL * @@ -747,9 +605,8 @@ iptablesAddForwardMasquerade(iptablesContext *ctx, * Returns 0 in case of success or an error code otherwise */ int -iptablesRemoveForwardMasquerade(iptablesContext *ctx, - const char *network, +iptablesRemoveForwardMasquerade(const char *network, const char *physdev) { - return iptablesForwardMasquerade(ctx, network, physdev, REMOVE); + return iptablesForwardMasquerade(network, physdev, REMOVE); } diff --git a/src/util/iptables.h b/src/util/iptables.h index 68d9e0d..8809d7d 100644 --- a/src/util/iptables.h +++ b/src/util/iptables.h @@ -22,72 +22,49 @@ #ifndef __QEMUD_IPTABLES_H__ #define __QEMUD_IPTABLES_H__ -typedef struct _iptablesContext iptablesContext; - -iptablesContext *iptablesContextNew (void); -void iptablesContextFree (iptablesContext *ctx); - -int iptablesAddTcpInput (iptablesContext *ctx, - const char *iface, +int iptablesAddTcpInput (const char *iface, int port); -int iptablesRemoveTcpInput (iptablesContext *ctx, - const char *iface, +int iptablesRemoveTcpInput (const char *iface, int port); -int iptablesAddUdpInput (iptablesContext *ctx, - const char *iface, +int iptablesAddUdpInput (const char *iface, int port); -int iptablesRemoveUdpInput (iptablesContext *ctx, - const char *iface, +int iptablesRemoveUdpInput (const char *iface, int port); -int iptablesAddForwardAllowOut (iptablesContext *ctx, - const char *network, +int iptablesAddForwardAllowOut (const char *network, const char *iface, const char *physdev); -int iptablesRemoveForwardAllowOut (iptablesContext *ctx, - const char *network, +int iptablesRemoveForwardAllowOut (const char *network, const char *iface, const char *physdev); -int iptablesAddForwardAllowRelatedIn(iptablesContext *ctx, - const char *network, +int iptablesAddForwardAllowRelatedIn(const char *network, const char *iface, const char *physdev); -int iptablesRemoveForwardAllowRelatedIn(iptablesContext *ctx, - const char *network, +int iptablesRemoveForwardAllowRelatedIn(const char *network, const char *iface, const char *physdev); -int iptablesAddForwardAllowIn (iptablesContext *ctx, - const char *network, +int iptablesAddForwardAllowIn (const char *network, const char *iface, const char *physdev); -int iptablesRemoveForwardAllowIn (iptablesContext *ctx, - const char *network, +int iptablesRemoveForwardAllowIn (const char *network, const char *iface, const char *physdev); -int iptablesAddForwardAllowCross (iptablesContext *ctx, - const char *iface); -int iptablesRemoveForwardAllowCross (iptablesContext *ctx, - const char *iface); +int iptablesAddForwardAllowCross (const char *iface); +int iptablesRemoveForwardAllowCross (const char *iface); -int iptablesAddForwardRejectOut (iptablesContext *ctx, - const char *iface); -int iptablesRemoveForwardRejectOut (iptablesContext *ctx, - const char *iface); +int iptablesAddForwardRejectOut (const char *iface); +int iptablesRemoveForwardRejectOut (const char *iface); -int iptablesAddForwardRejectIn (iptablesContext *ctx, - const char *iface); -int iptablesRemoveForwardRejectIn (iptablesContext *ctx, - const char *iface); +int iptablesAddForwardRejectIn (const char *iface); +int iptablesRemoveForwardRejectIn (const char *iface); -int iptablesAddForwardMasquerade (iptablesContext *ctx, - const char *network, +int iptablesAddForwardMasquerade (const char *network, const char *physdev); -int iptablesRemoveForwardMasquerade (iptablesContext *ctx, - const char *network, +int iptablesRemoveForwardMasquerade (const char *network, const char *physdev); #endif /* __QEMUD_IPTABLES_H__ */ -- 1.6.5.2

Daniel P. Berrange

1:08 p.m.

New subject: [libvirt] [PATCH 5/5] remove now unneeded iptablesContext

On Thu, Dec 10, 2009 at 11:27:55AM +0000, Mark McLoughlin wrote:

...

iptablesContext no longer contains any state, so we can drop it

* src/util/iptables.c, src/util/iptables.h: drop iptablesContext

* src/network/bridge_driver.c: update callers

* src/libvirt_private.syms: drop context new/free functions

Ordinarily I'd ACK this, but one of the things I want to try and do in the future is to move all the libvirt rules out of the main INPUT/FORWARD/OUPUT chains, and into sub-chains. I think that the iptablesContxt struct might be useful for this, so can we leave this patch out for now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Mark McLoughlin

1:17 p.m.

New subject: [libvirt] [PATCH 5/5] remove now unneeded iptablesContext

On Thu, 2009-12-10 at 12:08 +0000, Daniel P. Berrange wrote:

...

On Thu, Dec 10, 2009 at 11:27:55AM +0000, Mark McLoughlin wrote:

...
iptablesContext no longer contains any state, so we can drop it

* src/util/iptables.c, src/util/iptables.h: drop iptablesContext

* src/network/bridge_driver.c: update callers

* src/libvirt_private.syms: drop context new/free functions

Ordinarily I'd ACK this, but one of the things I want to try and do in the future is to move all the libvirt rules out of the main INPUT/FORWARD/OUPUT chains, and into sub-chains. I think that the iptablesContxt struct might be useful for this, so can we leave this patch out for now.

That could done e.g. by using "libvirt-INPUT", which again wouldn't need any state It's a very nice simplification, easy to re-instate, so I'd prefer to see it gone rather than for it to stick around under the guise of "we might need it in future". Look how long it took us to delete the lokkit code after we realized it was useless :) Cheers, Mark.

Daniel P. Berrange

1:04 p.m.

On Thu, Dec 10, 2009 at 11:27:51AM +0000, Mark McLoughlin wrote:

...

Currently, when we add iptables rules, we keep them on a list so that we can easily reload them on e.g. 'service libvirtd reload'.

However, we don't save this list to disk, so if libvirtd is restarted we lose the ability to reload the rules.

The fix is simple - just re-add the damn things on reload.

Note, we delete the rules before re-adding them, just like the current behaviour of iptRulesReload().

* src/network/bridge_driver.c: re-add the iptables rules on reload. --- src/network/bridge_driver.c | 30 ++++++++++++++++++++++++------ 1 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0342aa0..766f8cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -96,6 +96,8 @@ static int networkShutdownNetworkDaemon(virConnectPtr conn, struct network_driver *driver, virNetworkObjPtr network);

+static void networkReloadIptablesRules(struct network_driver *driver); + static struct network_driver *driverState = NULL;

@@ -291,12 +293,7 @@ networkReload(void) { &driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir); - - if (driverState->iptables) { - VIR_INFO0(_("Reloading iptables rules\n")); - iptablesReloadRules(driverState->iptables); - } - + networkReloadIptablesRules(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); return 0; @@ -812,6 +809,27 @@ networkRemoveIptablesRules(struct network_driver *driver, iptablesSaveRules(driver->iptables); }

+static void +networkReloadIptablesRules(struct network_driver *driver) +{ + unsigned int i; + + VIR_INFO0(_("Reloading iptables rules")); + + for (i = 0 ; i < driver->networks.count ; i++) { + virNetworkObjLock(driver->networks.objs[i]); + + if (virNetworkObjIsActive(driver->networks.objs[i])) { + networkRemoveIptablesRules(driver, driver->networks.objs[i]); + if (!networkAddIptablesRules(NULL, driver, driver->networks.objs[i])) { + /* failed to add but already logged */ + } + } + + virNetworkObjUnlock(driver->networks.objs[i]); + } +} + /* Enable IP Forwarding. Return 0 for success, -1 for failure. */ static int networkEnableIpForwarding(void)

5758

Age (days ago)

5758

Last active (days ago)

List overview

Download

10 comments

2 participants

participants (2)

Daniel P. Berrange
Mark McLoughlin