nwfilter: Check before removing and reinserting iptable base chains

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.

Closes: https://gitlab.com/libvirt/libvirt/-/issues/784
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com>

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c
index 067df6e612d82a35ff5a48ad15dc9f8aa9e24fd2..4578152670ababa22a589308a7c0dbf67212b31b 100644 (file)
--- 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 b78b1b7947f53bba4e478d8c778b7040d8de19a7..88ba15677d2f63296dd5df4d8178d40116e66820 100644 (file)
--- 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 (STRNEQ(args[0], "iptables") && STRNEQ(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;