[libvirt] [PATCH v2 2/2] nwfilter: don't reinstantiate rules if there is no need to

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