[libvirt] [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
Applied on top of [1] that restores reinstantiating filters on daemon reload. Note the fragile issue mentioned in ebiptablesDumpIstalledRules in respect to list of firewall tables we are using. I wonder can we instead of caching instantiate rules faster in the end? There is iptables-restore if we instantiate directly. And in case of firewalld mode why we instantiate filters via firewalld dbus interface after all? We use passthrough interface so looks like firewalld don't account our rules in any way. May be all we need is reloading rules on firewalld reload and always instantiate thru binaries? Then we can do things fast. Diff from v1 [2]: ------------ Approach is changed. Instead of checking whether applied filters changed or not (so we can miss firewall changes from outside) let's check that don't change both - rules we are going to apply and rules in firewall in comparion to previous instantiation. [1] [PATCH] nwfilter: intantiate filters on loading driver https://www.redhat.com/archives/libvir-list/2018-October/msg00787.html [2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html [3] [RFC] Faster libvirtd restart with nwfilter rules https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html which continues in https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html Nikolay Shirokovskiy (2): firewall: add dump function nwfilter: don't reinstantiate rules if there is no need to src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 387 +++++++++++++++++++++++++++++- src/util/virfirewall.c | 111 +++++++++ src/util/virfirewall.h | 3 + 4 files changed, 500 insertions(+), 2 deletions(-) -- 1.8.3.1
We are going to save this dump on applying rules so that on next applying we can compare dump of current rules with dump of previous rules. Which in turn will help us to decide wether to reinstantiate rules or not. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt_private.syms | 1 + src/util/virfirewall.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfirewall.h | 3 ++ 3 files changed, 115 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 335210c..612ad41 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1892,6 +1892,7 @@ virFileCacheSetPriv; # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; +virFirewallDumpRules; virFirewallFree; virFirewallNew; virFirewallRemoveRule; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index c786d76..ed7f9e6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -949,3 +949,114 @@ virFirewallApply(virFirewallPtr firewall) virMutexUnlock(&ruleLock); return ret; } + + +static void +virFirewallDumpRule(virBufferPtr buf, + const char *ifname, + virFirewallRulePtr rule) +{ + size_t i = 0; + size_t j, idx; + const char *concurrent = NULL; + + switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + concurrent = "--concurrent"; + break; + + case VIR_FIREWALL_LAYER_IPV4: + case VIR_FIREWALL_LAYER_IPV6: + concurrent = "-w"; + break; + + case VIR_FIREWALL_LAYER_LAST: + break; + } + + /* don't dump concurrent option */ + if (concurrent && rule->argsLen > 0 && STREQ(rule->args[0], concurrent)) + i++; + + /* calculate index of 'command' argument like -N or -A etc */ + idx = i; + if (i < rule->argsLen && STREQ(rule->args[i], "-t")) + idx += 2; + + /* Whitelist commands that can go to dump. These are -N and -A. + * Commands D, L, X, F are used only for cleanup purpuses and do + * not present result rules list. -I is only used by + * iptablesCreateBaseChainsFW to [re]insert common non binding specific + * rules. + */ + if (idx >= rule->argsLen || + (STRNEQ(rule->args[idx], "-N") && STRNEQ(rule->args[idx], "-A"))) + return; + + for (j = i; j < rule->argsLen; j++) + if (strstr(rule->args[j], ifname)) + break; + + /* We also need to filter common non binding specific rules originated + * from iptablesCreateBaseChainsFW - these are -N for libvirt-in etc. */ + if (j == rule->argsLen) + return; + + switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + virBufferAddLit(buf, "ebtables "); + break; + + case VIR_FIREWALL_LAYER_IPV4: + virBufferAddLit(buf, "iptables "); + break; + + case VIR_FIREWALL_LAYER_IPV6: + virBufferAddLit(buf, "ip6tables "); + break; + + case VIR_FIREWALL_LAYER_LAST: + break; + } + + /* dump default table explicitly */ + if (i < rule->argsLen && STRNEQ(rule->args[i], "-t")) + virBufferAddLit(buf, "-t filter "); + + for (; i < rule->argsLen; i++) { + virBufferAddStr(buf, rule->args[i]); + + if (i < rule->argsLen - 1) + virBufferAddLit(buf, " "); + } + + virBufferAddLit(buf, "\n"); +} + + +/* + * Dump rules for all transactions. Transaction rollbacks are not dumped. + * The order of rules in dump follows from order of transactions and + * order of rules in transactions. Only creation rules (-N, -A) are dumped. + * Example: + * + * ebtables -t nat -N libvirt-P-vme001c42fd388c + * ebtables -t nat -A libvirt-P-vme001c42fd388c -d 00:1c:42:fd:38:9d -j RETURN + * .. + */ +char* +virFirewallDumpRules(virFirewallPtr firewall, + const char *ifname) +{ + size_t i, j; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!firewall || firewall->err) + return NULL; + + for (i = 0; i < firewall->ngroups; i++) + for (j = 0; j < firewall->groups[i]->naction; j++) + virFirewallDumpRule(&buf, ifname, firewall->groups[i]->action[j]); + + return virBufferContentAndReset(&buf); +} diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index e024e88..bbbc561 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -115,6 +115,9 @@ void virFirewallStartRollback(virFirewallPtr firewall, int virFirewallApply(virFirewallPtr firewall); +char* virFirewallDumpRules(virFirewallPtr firewall, + const char *ifname); + void virFirewallSetLockOverride(bool avoid); VIR_DEFINE_AUTOPTR_FUNC(virFirewall, virFirewallFree) -- 1.8.3.1
This patch adds caching to rules instantiating in nwfilter. This instantiating can take much time as described in [1] so as we are lacking option to instantiate rules faster right now we can at least don't instantiate when we don't need to. And this is typical that between libvirtd restarts rules don't change in firewall so we don't need to reapply them. The most straightforward approach is just to compare rules to be instantiated against rules in firewall but this comparison is complicated (see issues below). So let's instead dump both rules we are going to instantiate and rules after instantiation every time we apply rules. So next time we are going to instantiate rules we can check whether rules in firewall changed or not and whether we are going to reinstantiate same rules or different. A few words on dumping rules we are going to instantiate. After filling firewall obj with rules we have several transactions some of them can have rollbacks. Among all these rules those that are in rollbacks and those that are aiming on preparation cleanup (-X, -F, -D and -L which helps to traverse tree on cleanup) are not significant and ommited in dump. * Comparison issues * - different rules order - rules arguments order can differ. For example: in: -A libvirt-out -m physdev --physdev-is-bridged --physdev-out vme001c42fd388c -g FO-vme001c42fd388c out: -A libvirt-out -m physdev --physdev-out vme001c42fd388c --physdev-is-bridged -g FO-vme001c42fd388c - result rule can have extra arguments. For example (here it looks like just explicit extension): in: -A FI-vme001c42fd388c -p tcp --dport 1000:3000 -j RETURN out: -A FI-vme001c42fd388c -p tcp -m tcp --dport 1000:3000 -j RETURN * References * [1] [RFC] Faster libvirtd restart with nwfilter rules https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 387 +++++++++++++++++++++++++++++- 1 file changed, 385 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 5be1c9b..4cd6b54 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -96,6 +96,8 @@ static bool newMatchState; #define MATCH_PHYSDEV_OUT_FW "-m", "physdev", "--physdev-is-bridged", "--physdev-out" #define MATCH_PHYSDEV_OUT_OLD_FW "-m", "physdev", "--physdev-out" +#define EBIPTABLES_CACHE_DIR (LOCALSTATEDIR "/run/libvirt/nwfilter-rules") + static int ebtablesRemoveBasicRules(const char *ifname); static int ebiptablesDriverInit(bool privileged); static void ebiptablesDriverShutdown(void); @@ -150,6 +152,10 @@ static char chainprefixes_host_temp[3] = { 0 }; +static virHashTablePtr ebiptables_cache_hits; + +static int ebiptablesTearNewRules(const char *ifname); + static int printVar(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, @@ -3393,6 +3399,339 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, } + +static int +ebiptablesCacheApplyNew(const char *ifname, + const char *dumpDriver) +{ + char *fileFirewall = NULL; + char *fileDriver = NULL; + char *fileDriverTmp = NULL; + int ret = -1; + + /* for tests */ + if (!ebiptables_cache_hits) + return 0; + + if (virAsprintf(&fileFirewall, "%s/%s.firewall", + EBIPTABLES_CACHE_DIR, ifname) < 0) + return -1; + + if (virAsprintf(&fileDriver, "%s/%s.driver", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + if (unlink(fileFirewall) < 0 && errno != ENOENT) { + virReportSystemError(errno, _("can not unlink file '%s'"), fileFirewall); + goto cleanup; + } + + if (unlink(fileDriver) < 0 && errno != ENOENT) { + virReportSystemError(errno, _("can not unlink file '%s'"), fileDriver); + goto cleanup; + } + + ret = 0; + + if (!dumpDriver) + goto cleanup; + + if (virAsprintfQuiet(&fileDriverTmp, "%s/%s.driver.tmp", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + if (virFileWriteStr(fileDriverTmp, dumpDriver, 0600) < 0) { + unlink(fileDriverTmp); + goto cleanup; + } + + rename(fileDriverTmp, fileDriver); + + cleanup: + VIR_FREE(fileFirewall); + VIR_FREE(fileDriver); + VIR_FREE(fileDriverTmp); + + return ret; +} + + +struct ebiptablesDumpData { + char *dump; + const char *ifname; + virFirewallLayer layer; +}; + + +static int +ebiptablesAppendInstalledRules(virFirewallPtr fw ATTRIBUTE_UNUSED, + const char *const *lines, + void *opaque) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + struct ebiptablesDumpData *data = opaque; + + virBufferAdd(&buf, data->dump, -1); + + while (*lines) { + if (strstr(*lines, data->ifname)) { + /* iptables/ip6tables do not add binary and table in front of rules + * thus let's do it ourselves */ + switch (data->layer) { + case VIR_FIREWALL_LAYER_IPV4: + if (!STRPREFIX(*lines, "iptables")) + virBufferAddLit(&buf, "iptables -t filter "); + break; + + case VIR_FIREWALL_LAYER_IPV6: + if (!STRPREFIX(*lines, "ip6tables")) + virBufferAddLit(&buf, "ip6tables -t filter "); + break; + + case VIR_FIREWALL_LAYER_ETHERNET: + case VIR_FIREWALL_LAYER_LAST: + break; + } + + virBufferAddStr(&buf, *lines); + virBufferAddLit(&buf, "\n"); + } + ++lines; + } + + data->dump = virBufferContentAndReset(&buf); + + return 0; +} + + +static char* +ebiptablesDumpIstalledRules(const char *ifname) +{ + virFirewallPtr fw; + struct ebiptablesDumpData data = { NULL, ifname, 0 }; + + /* Here we get fragile. We only check nat table in ebtables and filter + * table for both iptables and ip6tables because we don't use other tables + * right now. But if we start to then we are in danger as we will miss + * changes in that tables on cache checks. On the other hand always checking + * all the tables is overkill and has significant performance impact if + * number of VMs is large. Do we need to add some protection here? */ + + fw = virFirewallNew(); + virFirewallStartTransaction(fw, 0); + data.layer = VIR_FIREWALL_LAYER_ETHERNET; + virFirewallAddRuleFull(fw, data.layer, + false, ebiptablesAppendInstalledRules, &data, + "-t", "nat", "-L", "--Lx", NULL); + virFirewallApply(fw); + virFirewallFree(fw); + + fw = virFirewallNew(); + virFirewallStartTransaction(fw, 0); + data.layer = VIR_FIREWALL_LAYER_IPV4; + virFirewallAddRuleFull(fw, data.layer, + false, ebiptablesAppendInstalledRules, &data, + "-S", NULL); + virFirewallApply(fw); + virFirewallFree(fw); + + fw = virFirewallNew(); + virFirewallStartTransaction(fw, 0); + data.layer = VIR_FIREWALL_LAYER_IPV6; + virFirewallAddRuleFull(fw, data.layer, + false, ebiptablesAppendInstalledRules, &data, + "-S", NULL); + virFirewallApply(fw); + virFirewallFree(fw); + + return data.dump; +} + + +static void +ebiptablesCacheTearOld(const char *ifname, + bool success) +{ + char *fileFirewall = NULL; + char *fileFirewallTmp = NULL; + char *fileDriver = NULL; + char *dump = NULL; + bool error = true; + + /* for tests */ + if (!ebiptables_cache_hits) + return; + + if (virAsprintfQuiet(&fileDriver, "%s/%s.driver", + EBIPTABLES_CACHE_DIR, ifname) < 0) + return; + + /* We failed to tear old rules thus let's not cache so we can + * redo rules instantiantion and hopefully tear successfully + * next time + */ + if (!success) { + unlink(fileDriver); + VIR_FREE(fileDriver); + return; + } + + /* When applying new rules we can fail to save driver rules. + * We need both file for cache purpose thus there is no sense + * to save firewall rules. + */ + if (access(fileDriver, F_OK) < 0) + return; + + if (virAsprintfQuiet(&fileFirewall, "%s/%s.firewall", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + if (virAsprintfQuiet(&fileFirewallTmp, "%s/%s.firewall.tmp", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + if (!(dump = ebiptablesDumpIstalledRules(ifname))) + goto cleanup; + + if (virFileWriteStr(fileFirewallTmp, dump, 0600) < 0) { + unlink(fileFirewallTmp); + goto cleanup; + } + + if (rename(fileFirewallTmp, fileFirewall) < 0) + goto cleanup; + + error = false; + + cleanup: + if (error) + unlink(fileDriver); + + VIR_FREE(fileFirewall); + VIR_FREE(fileFirewallTmp); + VIR_FREE(fileDriver); + VIR_FREE(dump); +} + + +static void +ebiptablesCacheTearNew(const char *ifname) +{ + char *fileDriver = NULL; + + /* for tests */ + if (!ebiptables_cache_hits) + return; + + if (virAsprintfQuiet(&fileDriver, "%s/%s.driver", + EBIPTABLES_CACHE_DIR, ifname) < 0) + return; + + unlink(fileDriver); + VIR_FREE(fileDriver); + + /* We lost cache files at this point. We could preserve + * old cache files and restore them here but tearing + * new rules is error path so this is not really important. + */ +} + + +static void +ebiptablesCacheAllTeardown(const char *ifname) +{ + char *fileFirewall = NULL; + char *fileDriver = NULL; + + /* for tests */ + if (!ebiptables_cache_hits) + return; + + if (virAsprintf(&fileFirewall, "%s/%s.firewall", + EBIPTABLES_CACHE_DIR, ifname) < 0) + return; + + if (virAsprintf(&fileDriver, "%s/%s.driver", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + unlink(fileFirewall); + unlink(fileDriver); + + cleanup: + VIR_FREE(fileFirewall); + VIR_FREE(fileDriver); +} + + +static bool +ebiptablesCacheIsHit(const char *ifname, + const char *dumpDriver) +{ + char *fileDriver = NULL; + char *fileFirewall = NULL; + char *dumpDriverCached = NULL; + char *dumpFirewallCached = NULL; + char *dumpFirewall = NULL; + bool ret = false; + + /* for tests */ + if (!ebiptables_cache_hits) + return false; + + /* just to be sure we don't have remnaints from previous operations */ + virHashRemoveEntry(ebiptables_cache_hits, ifname); + + if (!dumpDriver) + return false; + + if (virAsprintfQuiet(&fileDriver, "%s/%s.driver", + EBIPTABLES_CACHE_DIR, ifname) < 0) + return false; + + if (virAsprintfQuiet(&fileFirewall, "%s/%s.firewall", + EBIPTABLES_CACHE_DIR, ifname) < 0) + goto cleanup; + + if (virFileReadAllQuiet(fileDriver, 1024 * 1024, &dumpDriverCached) < 0) { + if (errno == EOVERFLOW) + VIR_WARN("nwfilter rules cache files exceeds limit"); + + goto cleanup; + } + + if (virFileReadAllQuiet(fileFirewall, 1024 * 1024, &dumpFirewallCached) < 0) { + if (errno == EOVERFLOW) + VIR_WARN("nwfilter rules cache files exceeds limit"); + + goto cleanup; + } + + if (!(dumpFirewall = ebiptablesDumpIstalledRules(ifname))) + goto cleanup; + + if (STRNEQ(dumpFirewall, dumpFirewallCached) || + STRNEQ(dumpDriver, dumpDriverCached)) + goto cleanup; + + if (virHashAddEntry(ebiptables_cache_hits, ifname, (void *) 1) < 0) + goto cleanup; + + ret = true; + + cleanup: + VIR_FREE(fileDriver); + VIR_FREE(fileFirewall); + VIR_FREE(dumpFirewall); + VIR_FREE(dumpFirewallCached); + VIR_FREE(dumpDriverCached); + + return ret; +} + + static int ebiptablesApplyNewRules(const char *ifname, virNWFilterRuleInstPtr *rules, @@ -3408,6 +3747,7 @@ ebiptablesApplyNewRules(const char *ifname, char *errmsg = NULL; struct ebtablesSubChainInst **subchains = NULL; size_t nsubchains = 0; + char *dumpDriver = NULL; int ret = -1; if (!chains_in_set || !chains_out_set) @@ -3592,8 +3932,20 @@ ebiptablesApplyNewRules(const char *ifname, ebtablesRemoveTmpRootChainFW(fw, true, ifname); ebtablesRemoveTmpRootChainFW(fw, false, ifname); - if (virFirewallApply(fw) < 0) - goto cleanup; + dumpDriver = virFirewallDumpRules(fw, ifname); + + if (!ebiptablesCacheIsHit(ifname, dumpDriver)) { + if (virFirewallApply(fw) < 0) + goto cleanup; + + if (ebiptablesCacheApplyNew(ifname, dumpDriver) < 0) { + ebiptablesTearNewRules(ifname); + goto cleanup; + } + + } else { + VIR_DEBUG("Skip rules reinstantiating for %s", ifname); + } ret = 0; @@ -3604,6 +3956,7 @@ ebiptablesApplyNewRules(const char *ifname, virFirewallFree(fw); virHashFree(chains_in_set); virHashFree(chains_out_set); + VIR_FREE(dumpDriver); VIR_FREE(errmsg); return ret; @@ -3633,12 +3986,20 @@ ebiptablesTearNewRules(const char *ifname) virFirewallPtr fw = virFirewallNew(); int ret = -1; + if (virHashLookup(ebiptables_cache_hits, ifname)) { + virHashRemoveEntry(ebiptables_cache_hits, ifname); + return 0; + } + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); ebiptablesTearNewRulesFW(fw, ifname); ret = virFirewallApply(fw); virFirewallFree(fw); + + ebiptablesCacheTearNew(ifname); + return ret; } @@ -3648,6 +4009,11 @@ ebiptablesTearOldRules(const char *ifname) virFirewallPtr fw = virFirewallNew(); int ret = -1; + if (virHashLookup(ebiptables_cache_hits, ifname)) { + virHashRemoveEntry(ebiptables_cache_hits, ifname); + return 0; + } + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); @@ -3667,6 +4033,9 @@ ebiptablesTearOldRules(const char *ifname) ret = virFirewallApply(fw); virFirewallFree(fw); + + ebiptablesCacheTearOld(ifname, ret == 0); + return ret; } @@ -3686,6 +4055,9 @@ ebiptablesAllTeardown(const char *ifname) virFirewallPtr fw = virFirewallNew(); int ret = -1; + virHashRemoveEntry(ebiptables_cache_hits, ifname); + ebiptablesCacheAllTeardown(ifname); + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); ebiptablesTearNewRulesFW(fw, ifname); @@ -3826,6 +4198,15 @@ ebiptablesDriverInit(bool privileged) if (ebiptablesDriverProbeStateMatch() < 0) return -1; + if (virFileMakePathWithMode(EBIPTABLES_CACHE_DIR, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create cache directory '%s'"), + EBIPTABLES_CACHE_DIR); + return -1; + } + + if (!(ebiptables_cache_hits = virHashCreate(100, NULL))) + return -1; + ebiptables_driver.flags = TECHDRV_FLAG_INITIALIZED; return 0; @@ -3835,5 +4216,7 @@ ebiptablesDriverInit(bool privileged) static void ebiptablesDriverShutdown(void) { + virHashFree(ebiptables_cache_hits); + ebiptables_driver.flags = 0; } -- 1.8.3.1
participants (1)
-
Nikolay Shirokovskiy