On 3/11/19 11:05 AM, Daniel P. Berrangé wrote:
On Mon, Mar 11, 2019 at 09:37:52AM +0100, Michal Privoznik wrote:
The way this function works is that for both iptables and
ip6tables (or their firewalld friends) and for every table
("filter", "nat", "mangle") it lists chains defined for the table
and then calls iptablesPrivateChainCreate() over the list. The
callback is then supposed to find libvirt private chains and if
not found create rules to add them. So far so good. Problem is if
one of the tables doesn't exist (e.g. due to a module missing).
For instance, on my system I don't have CONFIG_IP6_NF_MANGLE
enabled therefore I'm lacking "mangle" table for ip6tables. This
means that the whole operation of setting up private chains fails
because the whole transaction is run as "do not ignore errors".
The solution is to have two transactions, the first one which
just lists chains can run ignoring errors, and the second one
which then installs the private chains will run normally.
This is just going to push the failure to a later point.
Is it? I mean sure, if you have some nwfilters that will apply some
rules to the missing table. But if you don't have such filters?
This causes iptablesPrivateChainCreate() to skip setup of the
libvirt global chain, so when we try to add rules to the libvirt
global chain later we'll find it doesn't exist.
I my limited setup, where I use the default network (which has just IPv4
enabled) and one domain using that network, no nwfilters this patch
fixes the problem for me and I don't see any subsequent errors. I don't
see any appealing argument to require IPv6 support in that case.
In the code, this approach is pushed to another level - every
table for which private chains are created is run as a separate
transaction. The reason is that it saves us one more variable
where we would track if the second transaction was started
already or not; and also, it doesn't matter.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/util/viriptables.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/util/viriptables.c b/src/util/viriptables.c
index d67b640a3b..ea24b03ec8 100644
--- a/src/util/viriptables.c
+++ b/src/util/viriptables.c
@@ -101,6 +101,16 @@ iptablesPrivateChainCreate(virFirewallPtr fw,
tmp++;
}
+ /* This function is running in the context of the very first transaction,
+ * which does nothing more than just lists current tables and chains. But
+ * since some tables might not be there (e.g. because of a module missing),
+ * the transaction is run with IGNORE_ERRORS flag. But obviously, we don't
+ * want to ignore errors here, where we are constructing our own chains and
+ * rules. The only way to resolve this is to start a new transaction so
+ * that all those AddRule() calls below add rules to new transaction/group.
+ */
+ virFirewallStartTransaction(fw, 0);
+
for (i = 0; i < data->nchains; i++) {
const char *from;
if (!virHashLookup(chains, data->chains[i].child)) {
@@ -160,7 +170,7 @@ iptablesSetupPrivateChains(void)
fw = virFirewallNew();
- virFirewallStartTransaction(fw, 0);
+ virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
This applies to all the chains, so if someone is missing more builtin
top level chains we'll potentially fail to create any of our global
chains which feels pretty undesirable to me.
Hm.. maybe I'm missing something, but when I was debugging this I saw
IGNORE_ERRORS being applied only for those "rules" [1] from the first
group/transaction. Any other rules that the callback added were run
normally, without ignoring errors.
Why shouldn't we just consider that missing kernel build option to be
a user bug ?
Well, as I say above, if you only have IPv4 network then the current
code is going to fail the moment it gets to the first IPv6 "rule" [1].
But if we decide to require IPv6, then we need to document that.
Michal
1: I say rules, but they aren't rules really, they merely just list chains.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
