On 4/8/26 15:23, Dion Bosschieter wrote:
On 4/7/26 15:41, Daniel P. Berrangé wrote:
On Fri, Apr 03, 2026 at 08:24:42PM +0200, Dion Bosschieter via Devel wrote:
On 4/2/26 06:29, Laine Stump wrote:
On 4/1/26 3:34 AM, Dion Bosschieter wrote:
Change the nwfilter driver loading mechanism to read from nwfilter.conf. By default, it will use the nftables driver, which follows the firewall_backend bridge driver config logic.
I think it should initially default to the iptables driver, just so nobody gets a surprise when they upgrade if there is any incompatibility at all - this is what we did when the nftables backend was added to the virtual network driver, and there are some distros that still keep their default set to iptables (due to interoperability problems with, e.g. docker using iptables rule with a default action of "reject" (or was it "deny"))
Should I do that by setting the aug file to the static value "iptables", asking because I think that the meson firewall_backend_priority is nftables by default, (which gets reused from the network driver) when:
%if 0%{?rhel} >= 10 || 0%{?fedora} %define prefer_nftables 1 %define firewall_backend_priority nftables,iptables
I'll check and update the nwfilter.conf reading, defaulting to ebiptables if no driver has been set.
BTW, this reminds me of the topic of "what happens when someone restarts nwfilterd after switching the setting of firewall_backend?" I could just build the patches, install, and find out for myself, but it's after midnight so I'll do the lazy thing and ask :-). It's important to keep track somewhere of whether the *previous* run of the daemon had loaded iptables or nftables rules. (for the network driver I did it in the status XML of each network).
Currently it doesn't do a cleanup when a user switches to a different nwfilter driver. When that happens there will be leftover firewall rules in either nftables or eb/iptables.
Can you share some examples of where that gets set and loaded?
I could then also run dropAllRules on the "old" driver so that when a user chooses the nftables driver, the old ebiptables rules get cleaned up.
It will require initializing both drivers and when the driver from status != current driver, we can do a cleanup via prev_driver.allTeardown(ifname)
I wonder if there is a place to store such a state/status, as I don't think there is a filter object per domain, it could be part of the filter itself or be stored global.
IIUC, Lanie is suggesting that we need to store state in /var/run/libvirt/nwfilter/.
We have a "nwfilter binding" object in the API which associates VM NICs and network filters. At the simplest level we could just record which backend was used for each binding, so we know which teardown codepath to invoke.
This indeed sounds like the simplest solution, I don't think we need a more fine grained solution, at least for now.
I wrote a patch which implements what you describe here, please take a look at which function is called where and if you agree with that. Im not sure if it is "correct" to call the virNWFilterBindingObjSave function inside nwfilter_gentech_driver.c and do the driver switch detection inside nwfilter_gentech_driver.c as well.
Just friendly bump on the last mail in this thread, I wonder if can get a review of the proposed solution. I can also include it in a new commit and send the series again as a v7.
diff --git a/src/conf/schemas/nwfilterbinding.rng b/src/conf/schemas/ nwfilterbinding.rng index c91312b09d..16ed745e0f 100644 --- a/src/conf/schemas/nwfilterbinding.rng +++ b/src/conf/schemas/nwfilterbinding.rng @@ -44,6 +44,13 @@ <element name="filterref"> <ref name="filterref-node-attributes"/> </element> + + <optional> + <element name="backend"> + <attribute name="name"/> + <empty/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/ virnwfilterbindingdef.c index fe45c84347..a91a54032c 100644 --- a/src/conf/virnwfilterbindingdef.c +++ b/src/conf/virnwfilterbindingdef.c @@ -26,7 +26,6 @@ #include "virnwfilterbindingdef.h" #include "viruuid.h"
- #define VIR_FROM_THIS VIR_FROM_NWFILTER
void @@ -143,8 +142,18 @@ virNWFilterBindingDefParseXML(xmlXPathContextPtr ctxt)
node = virXPathNode("./filterref", ctxt); if (node && - !(ret->filterparams = virNWFilterParseParamAttributes(node))) + !(ret->filterparams = virNWFilterParseParamAttributes(node))) { goto cleanup; + } + + if (virXPathNode("./backend", ctxt)) { + ret->backend = virXPathString("string(./backend/@name)", ctxt); + if (!ret->backend) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no firewall backend name")); + goto cleanup; + } + }
return ret;
@@ -211,6 +220,7 @@ virNWFilterBindingDefFormatBuf(virBuffer *buf, if (virNWFilterFormatParamAttributes(buf, def->filterparams, def-
filter) < 0) return -1;
+ virBufferAsprintf(buf, "<backend name='%s'/>", def->backend); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</filterbinding>\n");
diff --git a/src/conf/virnwfilterbindingdef.h b/src/conf/ virnwfilterbindingdef.h index 272ad686a0..16465df56d 100644 --- a/src/conf/virnwfilterbindingdef.h +++ b/src/conf/virnwfilterbindingdef.h @@ -36,6 +36,7 @@ struct _virNWFilterBindingDef { virMacAddr mac; char *filter; GHashTable *filterparams; + char *backend; };
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/ nwfilter_driver.c index 66f5fa7c18..9b981ddcbf 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -725,6 +725,7 @@ nwfilterBindingCreateXML(virConnectPtr conn, def = virNWFilterBindingDefParse(xml, NULL, flags); if (!def) return NULL; + def->backend = g_strdup(virFirewallBackendTypeToString(cfg-
firewallBackend));
if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0) goto cleanup; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/ nwfilter_gentech_driver.c index 29f80a8677..9dc99e848b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -70,6 +70,20 @@ int virNWFilterTechDriversInit(bool privileged, virNWFilterDriverConfig *config) }
+static virNWFilterTechDriver * +virNWFilterTechDriversByName(const char *firewallBackend) +{ + size_t i = 0; + + for (i = 0; i < G_N_ELEMENTS(filter_tech_drivers); i++) { + if (STREQ(filter_tech_drivers[i]->name, firewallBackend)) + return filter_tech_drivers[i]; + } + + return 0; +} + + void virNWFilterTechDriversShutdown(void) { size_t i = 0; @@ -786,6 +800,33 @@ virNWFilterRollbackUpdateFilter(virNWFilterBindingDef *binding) return techdriver->tearNewRules(binding->portdevname); }
config->firewallBackend));
+static int +virNWFilterSwitchTechDriver(virNWFilterDriverState *driver, + virNWFilterBindingObj *binding, + virNWFilterBindingDef *def) +{ + int ret = 0; + virNWFilterTechDriver *oldTechDriver = virNWFilterTechDriversByName(def->backend); + g_autoptr(virNWFilterDriverConfig) cfg = virNWFilterDriverGetConfig(driver); + + VIR_DEBUG("Detected change in nwfilter firewall backend '%1$s' -> '%2$s', cleaning up '%1$s'", + def->backend, + virFirewallBackendTypeToString(driver->config->firewallBackend)); + + // first we'll apply the rules with the new tech driver + if ((ret = virNWFilterInstantiateFilter(driver, def)) < 0) + return ret; + + // then we'll cleanup the rules from the old driver + if ((ret = oldTechDriver->allTeardown(def->portdevname)) < 0) + return ret; + + // update binding backend + def->backend = g_strdup(virFirewallBackendTypeToString(driver- + + // save changed binding xml file + return virNWFilterBindingObjSave(binding, cfg->bindingDir); +}
static int virNWFilterTearOldFilter(virNWFilterBindingDef *binding) @@ -854,40 +895,49 @@ enum {
portdevname, step); + virNWFilterBindingDef *def = virNWFilterBindingObjGetDef(binding);
static int virNWFilterBuildOne(virNWFilterDriverState *driver, - virNWFilterBindingDef *binding, + virNWFilterBindingObj *binding, GHashTable *skipInterfaces, int step) { bool skipIface; int ret = 0; - VIR_DEBUG("Building filter for portdev=%s step=%d", binding- + + VIR_DEBUG("Building filter for portdev=%s step=%d", def-
portdevname, step);
switch (step) { case STEP_APPLY_NEW: ret = virNWFilterUpdateInstantiateFilter(driver, - binding, + def, &skipIface); if (ret == 0 && skipIface) { /* filter tree unchanged -- no update needed */ ret = virHashAddEntry(skipInterfaces, - binding->portdevname, + def->portdevname, (void *)~0); } break; - case STEP_ROLLBACK: - if (!virHashLookup(skipInterfaces, binding->portdevname)) - ret = virNWFilterRollbackUpdateFilter(binding); + if (!virHashLookup(skipInterfaces, def->portdevname)) + ret = virNWFilterRollbackUpdateFilter(def); break; - case STEP_SWITCH: - if (!virHashLookup(skipInterfaces, binding->portdevname)) - ret = virNWFilterTearOldFilter(binding); + if (!virHashLookup(skipInterfaces, def->portdevname)) + ret = virNWFilterTearOldFilter(def); break; - case STEP_APPLY_CURRENT: - ret = virNWFilterInstantiateFilter(driver, - binding); + // prev is always ebiptables if unset + if (def->backend == NULL) + def->backend = g_strdup(ebiptables_driver.name); + + // detect change in backend + if (STRNEQ(def->backend, + virFirewallBackendTypeToString(driver->config->firewallBackend))) { + ret = virNWFilterSwitchTechDriver(driver, binding, def); + } else { + ret = virNWFilterInstantiateFilter(driver, def); + } + break; }
@@ -905,9 +955,8 @@ static int virNWFilterBuildIter(virNWFilterBindingObj *binding, void *opaque) { struct virNWFilterBuildData *data = opaque; - virNWFilterBindingDef *def = virNWFilterBindingObjGetDef(binding);
- return virNWFilterBuildOne(data->driver, def, + return virNWFilterBuildOne(data->driver, binding, data->skipInterfaces, data->step); }