[PATCH v2 0/1] nwfilter: Check before removing and reinserting iptable base chains
Thanks for the feedback. I've applied the iptables -L with a callback handler. The callback handler then decides whether or not to create the base chains. I changed the commit message from "nwfilter: Avoid firewall hole during VM startup by checking rule presence" to "nwfilter: Check before removing and reinserting iptable base chains". I also edited nwfilterxml2firewalltest, so it pretends that currently there are no chains and I changed the expected order of the commonRules. Dion Bosschieter (1): nwfilter: Check before removing and reinserting iptable base chains src/nwfilter/nwfilter_ebiptables_driver.c | 203 +++++++++++++--------- tests/nwfilterxml2firewalltest.c | 58 +++++-- 2 files changed, 163 insertions(+), 98 deletions(-) -- 2.43.0
Upon VM bootstrapping (start,restore,incoming migration) iptablesCreateBaseChainsFW is called and unconditionally deletes and reinserts top-level firewall chain jumps (e.g. INPUT, FORWARD rules). This briefly allows packets to continue, allowing packets through until the base chain iptables -I commands run. This commit ensures that the base chains are only created once per layer (IPV4/IPV6) and checks whether the expected rules already exist using `iptables -L`. If they do, no delete/insert operations are performed. By checking for the existence of rules we can prevent more rules from being created if they already exist. Possibly speeding up nwfilter by reducing the amount of iptable commands it executes. This however is not part of this patch. Solves: https://gitlab.com/libvirt/libvirt/-/issues/784 Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 203 +++++++++++++--------- tests/nwfilterxml2firewalltest.c | 58 +++++-- 2 files changed, 163 insertions(+), 98 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 067df6e612..18cb870fba 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -131,6 +131,25 @@ static char chainprefixes_host_temp[3] = { 0 }; +typedef struct { + const char *chain; + const char *position; + const char *targetChain; +} iptablesBaseChainFW; + +typedef struct { + const char *ifname; + int nrules; + virNWFilterRuleInst **rules; +} chainCreateCallbackData; + +static iptablesBaseChainFW fw_base_chains[] = { + {"FORWARD", "1", VIRT_IN_CHAIN}, + {"FORWARD", "2", VIRT_OUT_CHAIN}, + {"FORWARD", "3", VIRT_IN_POST_CHAIN}, + {"INPUT", "1", HOST_IN_CHAIN}, +}; + static int printVar(virNWFilterVarCombIter *vars, char *buf, int bufsize, @@ -398,46 +417,6 @@ ebtablesHandleEthHdr(virFirewall *fw, /************************ iptables support ************************/ - -static void -iptablesCreateBaseChainsFW(virFirewall *fw, - virFirewallLayer layer) -{ - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-N", VIRT_IN_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-N", VIRT_OUT_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-N", VIRT_IN_POST_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-N", HOST_IN_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-D", "FORWARD", "-j", VIRT_IN_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-D", "FORWARD", "-j", VIRT_OUT_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-D", "FORWARD", "-j", VIRT_IN_POST_CHAIN, NULL); - virFirewallAddCmdFull(fw, layer, - true, NULL, NULL, - "-D", "INPUT", "-j", HOST_IN_CHAIN, NULL); - virFirewallAddCmd(fw, layer, - "-I", "FORWARD", "1", "-j", VIRT_IN_CHAIN, NULL); - virFirewallAddCmd(fw, layer, - "-I", "FORWARD", "2", "-j", VIRT_OUT_CHAIN, NULL); - virFirewallAddCmd(fw, layer, - "-I", "FORWARD", "3", "-j", VIRT_IN_POST_CHAIN, NULL); - virFirewallAddCmd(fw, layer, - "-I", "INPUT", "1", "-j", HOST_IN_CHAIN, NULL); -} - - static void iptablesCreateTmpRootChainFW(virFirewall *fw, virFirewallLayer layer, @@ -3148,6 +3127,7 @@ iptablesCheckBridgeNFCallEnabled(bool isIPv6) } } + /* * Given a filtername determine the protocol it is used for evaluating * We do prefix-matching to determine the protocol. @@ -3202,6 +3182,104 @@ iptablesRuleInstCommand(virFirewall *fw, } +static int +iptablesHandleCreateChainAndRules(virFirewall *fw, + virFirewallLayer layer, + const char *const *lines, + void *opaque) +{ + size_t i, j; + static bool baseChainDefined[G_N_ELEMENTS(fw_base_chains)] = { false }; + chainCreateCallbackData *cbdata = opaque; + bool isIPv6 = layer == VIR_FIREWALL_LAYER_IPV6; + + iptablesUnlinkTmpRootChainsFW(fw, layer, cbdata->ifname); + iptablesRemoveTmpRootChainsFW(fw, layer, cbdata->ifname); + + // parse iptables -L output to see if base chains exist + for (i = 0; lines[i] != NULL; i++) { + if (!STRPREFIX(lines[i], "Chain ")) { + // line doesn't start with Chain + continue; + }; + + VIR_DEBUG("Considering chain for comparison '%s'", lines[i]); + + for (j = 0; j < G_N_ELEMENTS(fw_base_chains); j++) { + // if chain matches basechain + if (STRPREFIX(lines[i]+6, fw_base_chains[j].targetChain)) { + VIR_DEBUG("Found chain '%s'", fw_base_chains[j].targetChain); + baseChainDefined[j] = true; + } + } + } + + // go through parsing results and add basechain commands if necessary + for (i = 0; i < G_N_ELEMENTS(fw_base_chains); i++) { + if (!baseChainDefined[i]) { + VIR_DEBUG("Defining base chain '%s'", fw_base_chains[i].targetChain); + + virFirewallAddCmdFull(fw, layer, + true, NULL, NULL, + "-N", fw_base_chains[i].targetChain, NULL); + virFirewallAddCmdFull(fw, layer, + true, NULL, NULL, + "-D", fw_base_chains[i].chain, "-j", + fw_base_chains[i].targetChain, NULL); + virFirewallAddCmd(fw, layer, + "-I", fw_base_chains[i].chain, fw_base_chains[i].position, + "-j", fw_base_chains[i].targetChain, NULL); + } + } + + iptablesCreateTmpRootChainsFW(fw, layer, cbdata->ifname); + iptablesLinkTmpRootChainsFW(fw, layer, cbdata->ifname); + iptablesSetupVirtInPostFW(fw, layer, cbdata->ifname); + + for (i = 0; i < cbdata->nrules; i++) { + if (isIPv6 == false && + virNWFilterRuleIsProtocolIPv4(cbdata->rules[i]->def)) { + if (iptablesRuleInstCommand(fw, cbdata->ifname, cbdata->rules[i]) < 0) + goto cleanup; + } else if (isIPv6 && virNWFilterRuleIsProtocolIPv6(cbdata->rules[i]->def)) { + if (iptablesRuleInstCommand(fw, cbdata->ifname, cbdata->rules[i]) < 0) + goto cleanup; + } + } + + iptablesCheckBridgeNFCallEnabled(isIPv6); + + cleanup: + + return 0; +} + + +/** + * iptablesCreateChainsAndRules + * + * @fw: the firewall ruleset instance + * @layer: the firewall layer + * @cbdata: callback data struct which holds variables that + * the call back handler needs in order to create + * the base chain jumps and the dependant rules + * the ICMP rule from being created + * + * Run iptables -L and parse if chains already exist + * skips creation of base chain jumps if possible + * see handler in iptablesHandleCreateChainAndRules + */ +static void iptablesCreateChainsAndRules(virFirewall *fw, + virFirewallLayer layer, + chainCreateCallbackData *cbdata) +{ + virFirewallAddCmdFull(fw, layer, + false, iptablesHandleCreateChainAndRules, + (void *)cbdata, + "-L", NULL, NULL); +} + + static int ebtablesRuleInstCommand(virFirewall *fw, const char *ifname, @@ -3311,6 +3389,7 @@ ebiptablesApplyNewRules(const char *ifname, g_autofree ebtablesSubChainInst **subchains = NULL; size_t nsubchains = 0; int ret = -1; + chainCreateCallbackData chainCallbackData = {ifname, nrules, rules}; if (nrules) { g_qsort_with_data(rules, nrules, sizeof(rules[0]), @@ -3429,47 +3508,9 @@ ebiptablesApplyNewRules(const char *ifname, } if (haveIptables) { - iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); - iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); - - iptablesCreateBaseChainsFW(fw, VIR_FIREWALL_LAYER_IPV4); - iptablesCreateTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); - - iptablesLinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); - iptablesSetupVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname); - - for (i = 0; i < nrules; i++) { - if (virNWFilterRuleIsProtocolIPv4(rules[i]->def)) { - if (iptablesRuleInstCommand(fw, - ifname, - rules[i]) < 0) - goto cleanup; - } - } - - iptablesCheckBridgeNFCallEnabled(false); - } - - if (haveIp6tables) { - iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname); - iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname); - - iptablesCreateBaseChainsFW(fw, VIR_FIREWALL_LAYER_IPV6); - iptablesCreateTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname); - - iptablesLinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname); - iptablesSetupVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname); - - for (i = 0; i < nrules; i++) { - if (virNWFilterRuleIsProtocolIPv6(rules[i]->def)) { - if (iptablesRuleInstCommand(fw, - ifname, - rules[i]) < 0) - goto cleanup; - } - } - - iptablesCheckBridgeNFCallEnabled(true); + iptablesCreateChainsAndRules(fw, VIR_FIREWALL_LAYER_IPV4, &chainCallbackData); + } if (haveIp6tables) { + iptablesCreateChainsAndRules(fw, VIR_FIREWALL_LAYER_IPV6, &chainCallbackData); } if (virHashSize(chains_in_set) != 0) diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c index b78b1b7947..3da28521b0 100644 --- a/tests/nwfilterxml2firewalltest.c +++ b/tests/nwfilterxml2firewalltest.c @@ -67,6 +67,14 @@ static const char *commonRules[] = { "ebtables \\\n--concurrent \\\n-t nat \\\n-N libvirt-J-vnet0\n" "ebtables \\\n--concurrent \\\n-t nat \\\n-N libvirt-P-vnet0\n", + /* Querying existence of rules */ + "iptables \\\n-w \\\n-L\n", + "ip6tables \\\n-w \\\n-L\n", + + /* Inserting ebtables rules */ + "ebtables \\\n--concurrent \\\n-t nat \\\n-A PREROUTING \\\n-i vnet0 \\\n-j libvirt-J-vnet0\n" + "ebtables \\\n--concurrent \\\n-t nat \\\n-A POSTROUTING \\\n-o vnet0 \\\n-j libvirt-P-vnet0\n", + /* Dropping iptables rules */ "iptables \\\n-w \\\n-D libvirt-out \\\n-m physdev \\\n--physdev-is-bridged \\\n--physdev-out vnet0 \\\n-g FP-vnet0\n" "iptables \\\n-w \\\n-D libvirt-out \\\n-m physdev \\\n--physdev-out vnet0 \\\n-g FP-vnet0\n" @@ -81,16 +89,16 @@ static const char *commonRules[] = { /* Creating iptables chains */ "iptables \\\n-w \\\n-N libvirt-in\n" - "iptables \\\n-w \\\n-N libvirt-out\n" - "iptables \\\n-w \\\n-N libvirt-in-post\n" - "iptables \\\n-w \\\n-N libvirt-host-in\n" "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in\n" - "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n" - "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n" - "iptables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n" "iptables \\\n-w \\\n-I FORWARD 1 \\\n-j libvirt-in\n" + "iptables \\\n-w \\\n-N libvirt-out\n" + "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n" "iptables \\\n-w \\\n-I FORWARD 2 \\\n-j libvirt-out\n" + "iptables \\\n-w \\\n-N libvirt-in-post\n" + "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n" "iptables \\\n-w \\\n-I FORWARD 3 \\\n-j libvirt-in-post\n" + "iptables \\\n-w \\\n-N libvirt-host-in\n" + "iptables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n" "iptables \\\n-w \\\n-I INPUT 1 \\\n-j libvirt-host-in\n" "iptables \\\n-w \\\n-N FP-vnet0\n" "iptables \\\n-w \\\n-N FJ-vnet0\n" @@ -115,16 +123,16 @@ static const char *commonRules[] = { /* Creating ip6tables chains */ "ip6tables \\\n-w \\\n-N libvirt-in\n" - "ip6tables \\\n-w \\\n-N libvirt-out\n" - "ip6tables \\\n-w \\\n-N libvirt-in-post\n" - "ip6tables \\\n-w \\\n-N libvirt-host-in\n" "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in\n" - "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n" - "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n" - "ip6tables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n" "ip6tables \\\n-w \\\n-I FORWARD 1 \\\n-j libvirt-in\n" + "ip6tables \\\n-w \\\n-N libvirt-out\n" + "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n" "ip6tables \\\n-w \\\n-I FORWARD 2 \\\n-j libvirt-out\n" + "ip6tables \\\n-w \\\n-N libvirt-in-post\n" + "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n" "ip6tables \\\n-w \\\n-I FORWARD 3 \\\n-j libvirt-in-post\n" + "ip6tables \\\n-w \\\n-N libvirt-host-in\n" + "ip6tables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n" "ip6tables \\\n-w \\\n-I INPUT 1 \\\n-j libvirt-host-in\n" "ip6tables \\\n-w \\\n-N FP-vnet0\n" "ip6tables \\\n-w \\\n-N FJ-vnet0\n" @@ -134,10 +142,6 @@ static const char *commonRules[] = { "ip6tables \\\n-w \\\n-A libvirt-host-in \\\n-m physdev \\\n--physdev-in vnet0 \\\n-g HJ-vnet0\n" "ip6tables \\\n-w \\\n-D libvirt-in-post \\\n-m physdev \\\n--physdev-in vnet0 \\\n-j ACCEPT\n" "ip6tables \\\n-w \\\n-A libvirt-in-post \\\n-m physdev \\\n--physdev-in vnet0 \\\n-j ACCEPT\n", - - /* Inserting ebtables rules */ - "ebtables \\\n--concurrent \\\n-t nat \\\n-A PREROUTING \\\n-i vnet0 \\\n-j libvirt-J-vnet0\n" - "ebtables \\\n--concurrent \\\n-t nat \\\n-A POSTROUTING \\\n-o vnet0 \\\n-j libvirt-P-vnet0\n", }; @@ -342,6 +346,26 @@ static int testSetDefaultParameters(GHashTable *vars) return 0; } +static void +testCommandDryRunCallback(const char *const*args, + const char *const*env G_GNUC_UNUSED, + const char *input G_GNUC_UNUSED, + char **output, + char **error G_GNUC_UNUSED, + int *status, + void *opaque G_GNUC_UNUSED) +{ + if (!STREQ(args[0], "iptables") && !STREQ(args[0], "ip6tables")) { + return; + } + + // simulate an empty existing set rules + if (STREQ(args[1], "-w") && STREQ(args[2], "-L")) { + *output = g_strdup("Chain nothing\n"); + *status = EXIT_SUCCESS; + } +} + static int testCompareXMLToArgvFiles(const char *xml, const char *cmdline) { @@ -352,7 +376,7 @@ static int testCompareXMLToArgvFiles(const char *xml, int ret = -1; g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); - virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL); + virCommandSetDryRun(dryRunToken, &buf, true, true, testCommandDryRunCallback, NULL); if (testSetDefaultParameters(vars) < 0) goto cleanup; -- 2.43.0
On Wed, Jul 16, 2025 at 12:08:58PM +0200, Dion Bosschieter wrote:
Upon VM bootstrapping (start,restore,incoming migration) iptablesCreateBaseChainsFW is called and unconditionally deletes and reinserts top-level firewall chain jumps (e.g. INPUT, FORWARD rules). This briefly allows packets to continue, allowing packets through until the base chain iptables -I commands run.
This commit ensures that the base chains are only created once per layer (IPV4/IPV6) and checks whether the expected rules already exist using `iptables -L`. If they do, no delete/insert operations are performed.
By checking for the existence of rules we can prevent more rules from being created if they already exist. Possibly speeding up nwfilter by reducing the amount of iptable commands it executes. This however is not part of this patch.
Solves: https://gitlab.com/libvirt/libvirt/-/issues/784 Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 203 +++++++++++++--------- tests/nwfilterxml2firewalltest.c | 58 +++++-- 2 files changed, 163 insertions(+), 98 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 067df6e612..18cb870fba 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -131,6 +131,25 @@ static char chainprefixes_host_temp[3] = { +static int +iptablesHandleCreateChainAndRules(virFirewall *fw, + virFirewallLayer layer, + const char *const *lines, + void *opaque)
Mis-alignment of params
+{ + size_t i, j; + static bool baseChainDefined[G_N_ELEMENTS(fw_base_chains)] = { false }; + chainCreateCallbackData *cbdata = opaque; + bool isIPv6 = layer == VIR_FIREWALL_LAYER_IPV6; + + iptablesUnlinkTmpRootChainsFW(fw, layer, cbdata->ifname); + iptablesRemoveTmpRootChainsFW(fw, layer, cbdata->ifname); + + // parse iptables -L output to see if base chains exist
We prefer traditional /* ... */ comments over C++ style
+ for (i = 0; lines[i] != NULL; i++) { + if (!STRPREFIX(lines[i], "Chain ")) { + // line doesn't start with Chain + continue; + }; + @@ -342,6 +346,26 @@ static int testSetDefaultParameters(GHashTable *vars) return 0; }
+static void +testCommandDryRunCallback(const char *const*args, + const char *const*env G_GNUC_UNUSED, + const char *input G_GNUC_UNUSED, + char **output, + char **error G_GNUC_UNUSED, + int *status, + void *opaque G_GNUC_UNUSED) +{ + if (!STREQ(args[0], "iptables") && !STREQ(args[0], "ip6tables")) {
'ninja -C build test' complains that we need STRNEQ here instead. I've made all the minor changes, tested it functionally, and pushed to git. Thanks for spending the time to get this figured out. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Dion Bosschieter