From nobody Mon Feb 9 19:05:37 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of redhat.com designates 216.205.24.124 as permitted sender) client-ip=216.205.24.124; envelope-from=libvir-list-bounces@redhat.com; helo=us-smtp-delivery-124.mimecast.com; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 216.205.24.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1606188631; cv=none; d=zohomail.com; s=zohoarc; b=QF2nTNWPAP3YQTDmVS4g0OKFDBZnMPdpT+jCfFJa4qtl1DK0EWn2o4gOa48gL5fAMqLesKCacOzfAp0PrHUUMCyKO9I9fZirnz8fmRGNn2O37Cq5/G1D+kHVG7YnGm6h1aTbmZlPCZPT+3G2x952EfDzYVzyj81Bpnd2dYtIz40= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1606188631; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=IimdoBJvRaYw0dtj+RFlg2jPYq58U70pExLUu5J5QZk=; b=RdzdgnHZw8RbOaqlFWvgUKm7/Vp6Ee3kM6+t0OzoFsetZYM5i7bO3MCiU39aDk3QQUTJfmUQ4Dpu2VvCYB3MAKsqdBpP6quzMABm/gZICksAOyrtmZ4cE8nLpO53H8YGs2sqtxG/B7YF5gEvF4A/tZ5UhSQM9XXCGUNXgHvs4BY= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 216.205.24.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) header.from= Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.zohomail.com with SMTPS id 160618863170755.97611838266573; Mon, 23 Nov 2020 19:30:31 -0800 (PST) Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-327-VWilUVAFOrSovRwx7bPfDA-1; Mon, 23 Nov 2020 22:30:27 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 250955212; Tue, 24 Nov 2020 03:30:21 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DF85219C59; Tue, 24 Nov 2020 03:30:19 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id E9A401809C9F; Tue, 24 Nov 2020 03:30:15 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 0AO3UFrC007410 for ; Mon, 23 Nov 2020 22:30:15 -0500 Received: by smtp.corp.redhat.com (Postfix) id 545545D705; Tue, 24 Nov 2020 03:30:15 +0000 (UTC) Received: from vhost2.laine.org (ovpn-112-35.phx2.redhat.com [10.3.112.35]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1C4655D6A1 for ; Tue, 24 Nov 2020 03:30:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606188630; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=IimdoBJvRaYw0dtj+RFlg2jPYq58U70pExLUu5J5QZk=; b=EZRpBeIduMvcSCe+utTJvvhn/+mz7Im4vf/86btKSK2BJPuURWndA0J+Vh57ID5Z1uxBlQ gQsOZR+K615J3/AHxEtkVy8HQbSlg+eVZZofQfW43skMJXGxgLVotj/sMIo3ts1zZAvDpa IvcQ/walbsXl7bdQrMkueT3bYipOAF0= X-MC-Unique: VWilUVAFOrSovRwx7bPfDA-1 From: Laine Stump To: libvir-list@redhat.com Subject: [PATCH 8/8] util: call iptables directly rather than via firewalld Date: Mon, 23 Nov 2020 22:30:04 -0500 Message-Id: <20201124033004.1163126-9-laine@redhat.com> In-Reply-To: <20201124033004.1163126-1-laine@redhat.com> References: <20201124033004.1163126-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-loop: libvir-list@redhat.com X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=libvir-list-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @redhat.com) Content-Type: text/plain; charset="utf-8" When libvirt added support for firewalld, we were unable to use firewalld's higher level rules, because they weren't detailed enough and could not be applied to the iptables FORWARD or OUTPUT chains (only to the INPUT chain). Instead we changed our code so that rather than running the iptables/ip6tables/ebtables binaries ourselves, we would send these commands to firewalld as "passthrough commands", and firewalld would run the appropriate program on our behalf. This was done under the assumption that firewalld was somehow tracking all these rules, and that this tracking was benefitting proper operation of firewalld and the system in general. Several years later this came up in a discussion on IRC, and we learned from the firewalld developers that, in fact, adding iptables and ebtables rules with firewalld's passthrough commands actually has *no* advantage; firewalld doesn't keep track of these rules in any way, and doesn't use them to tailor the construction of its own rules. Meanwhile, users have been complaining for some time that whenever firewalld is restarted on a system with libvirt virtual networks and/or nwfilter rules active, the system logs would be flooded with warning messages whining that [lots of different rules] could not be deleted because they didn't exist. For example: firewalld[3536040]: WARNING: COMMAND_FAILED: '/usr/sbin/iptables -w10 -w --table filter --delete LIBVIRT_OUT --out-interface virbr4 --protocol udp --destination-port 68 --jump ACCEPT' failed: iptables: Bad rule (does a matching rule exist in that chain?). (See https://bugzilla.redhat.com/1790837 for many more examples and a discussion) Note that these messages are created by iptables, but are logged by firewalld - when an iptables/ebtables command fails, firewalld grabs whatever is in stderr of the program, and spits it out to the system log as a warning. We've requested that firewalld not do this (and instead leave it up to the calling application to do the appropriate logging), but this request has been respectfully denied. But combining the two problems above ( 1) firewalld doesn't do anything useful when you use it as a proxy to add/remove iptables rules, 2) firewalld often insists on logging lots of annoying/misleading/useless "error" messages when you use it as a proxy to remove iptables rules that don't already exist), leads to a solution - simply stop using firewalld to add and remove iptables rules. Instead, exec iptables/ip6tables/ebtables directly in the same way we do when firewalld isn't active. We still need to keep track of whether or not firewalld is active, as there are some things that must be done, e.g. we need to add some actual firewalld rules in the firewalld "libvirt" zone, and we need to take notice when firewalld restarts, so that we can reload all our rules. This patch doesn't remove the infrastructure that allows having different firewall backends that perform their functions in different ways, as that will very possibly come in handy in the future when we want to have an nftables direct backend, and possibly a "pure" firewalld backend (now that firewalld supports more complex rules, and can add those rules to the FORWARD and OUTPUT chains). Instead, it just changes the action when the selected backend is "firewalld" so that it adds rules directly rather than through firewalld, while leaving as much of the existing code intact as possible. In order for tests to still pass, virfirewalltest also had to be modified to behave in a different way (i.e. by capturing the generated commandline as it does for the DIRECT backend, rather than capturing dbus messages using a mocked dbus API). Signed-off-by: Laine Stump --- src/util/virfirewall.c | 13 +++++++++++-- tests/virfirewalltest.c | 30 ++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 307825331d..21dea3013a 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -640,7 +640,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, } =20 =20 -static int +static int G_GNUC_UNUSED virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, bool ignoreErrors, char **output) @@ -698,7 +698,16 @@ virFirewallApplyRule(virFirewallPtr firewall, return -1; break; case VIR_FIREWALL_BACKEND_FIREWALLD: - if (virFirewallApplyRuleFirewallD(rule, ignoreErrors, &output) < 0) + /* Since we are using raw iptables rules, there is no + * advantage to going through firewalld, so instead just add + * them directly rather that via dbus calls to firewalld. This + * has the useful side effect of eliminating extra unwanted + * warning messages in the system logs when trying to delete + * rules that don't exist (which is something that happens + * often when libvirtd is started, and *always* when firewalld + * is restarted) + */ + if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0) return -1; break; =20 diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index fa1838a499..2670eb1561 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -194,7 +194,8 @@ testFirewallSingleGroup(const void *opaque) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) virCommandSetDryRun(&cmdbuf, NULL, NULL); else fwBuf =3D &cmdbuf; @@ -247,7 +248,8 @@ testFirewallRemoveRule(const void *opaque) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) virCommandSetDryRun(&cmdbuf, NULL, NULL); else fwBuf =3D &cmdbuf; @@ -307,7 +309,8 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) virCommandSetDryRun(&cmdbuf, NULL, NULL); else fwBuf =3D &cmdbuf; @@ -394,7 +397,8 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_U= NUSED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwBuf =3D &cmdbuf; @@ -462,7 +466,8 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UN= USED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwBuf =3D &cmdbuf; @@ -527,7 +532,8 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwBuf =3D &cmdbuf; @@ -590,7 +596,8 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UN= USED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwError =3D true; @@ -669,7 +676,8 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUS= ED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwBuf =3D &cmdbuf; @@ -756,7 +764,8 @@ testFirewallChainedRollback(const void *opaque G_GNUC_U= NUSED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL); } else { fwBuf =3D &cmdbuf; @@ -951,7 +960,8 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) if (virFirewallSetBackend(data->tryBackend) < 0) goto cleanup; =20 - if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT) { + if (data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_DIRECT || + data->expectBackend =3D=3D VIR_FIREWALL_BACKEND_FIREWALLD) { virCommandSetDryRun(&cmdbuf, testFirewallQueryHook, NULL); } else { fwBuf =3D &cmdbuf; --=20 2.28.0