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 <dionbosschie...@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 :|
