[libvirt] [PATCH] iptablesSetupPrivateChains: Be forgiving if a table does not exist
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. 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@redhat.com> --- 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); for (i = 0; i < ARRAY_CARDINALITY(data); i++) virFirewallAddRuleFull(fw, data[i].layer, -- 2.19.2
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. 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.
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@redhat.com> --- 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. Why shouldn't we just consider that missing kernel build option to be a user bug ?
for (i = 0; i < ARRAY_CARDINALITY(data); i++) virFirewallAddRuleFull(fw, data[i].layer,
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 :|
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@redhat.com> --- 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.
On Mon, Mar 11, 2019 at 11:27:33AM +0100, Michal Privoznik wrote:
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?
NB, this code is not used by nwfilter at all. It is just for the firewall rules related to the virtual network.
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.
Ah, I forgot we still don't have the IPv6 NAT support enabled in libvirt.
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.
What I mean is that this transaction is checking the filter, nat and mangle tables of both ipv4 and ipv6. You have a missing mangle table for ipv6, but this "ignore errors" policy means we'll even ignore the missing "filter" table for ipv4 for example which is something we have previously considered mandatory. We will still get a failure later when the network is started though I guess.
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.
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 :|
On 3/11/19 11:43 AM, Daniel P. Berrangé wrote:
What I mean is that this transaction is checking the filter, nat and mangle tables of both ipv4 and ipv6. You have a missing mangle table for ipv6, but this "ignore errors" policy means we'll even ignore the missing "filter" table for ipv4 for example which is something we have previously considered mandatory.
We will still get a failure later when the network is started though I guess.
I know, and to me that's acceptable. It will not be any worse with this patch. Only better. Because right now we fail even for IPv6 even though you might not use it. But fair enough. I'll post a patch documenting that IPv6 tables are required. Michal
On Mon, Mar 11, 2019 at 12:55:33PM +0100, Michal Privoznik wrote:
On 3/11/19 11:43 AM, Daniel P. Berrangé wrote:
What I mean is that this transaction is checking the filter, nat and mangle tables of both ipv4 and ipv6. You have a missing mangle table for ipv6, but this "ignore errors" policy means we'll even ignore the missing "filter" table for ipv4 for example which is something we have previously considered mandatory.
We will still get a failure later when the network is started though I guess.
I know, and to me that's acceptable. It will not be any worse with this patch. Only better. Because right now we fail even for IPv6 even though you might not use it.
Yes, my main real concern is how well this works from POV of debugging failures users report. With the current behaviour we'll see an error that the main iptables mangle chain doesn't exist, which is accurate. With the new behaviour we'll see an error that the libvirt chain doesn't exist. It isn't as obvious then that the problem is actually the kernel missing its built-in chain, rather than a bug in libvirt. Perhaps we could just issue a VIR_WARN in startup if we see one of the built-in chains missing. That would encourage people to enable all the kernel features, without forcing it & help with diagnosis.
But fair enough. I'll post a patch documenting that IPv6 tables are required.
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 :|
On Mon, 2019-03-11 at 12:55 +0100, Michal Privoznik wrote:
On 3/11/19 11:43 AM, Daniel P. Berrangé wrote:
What I mean is that this transaction is checking the filter, nat and mangle tables of both ipv4 and ipv6. You have a missing mangle table for ipv6, but this "ignore errors" policy means we'll even ignore the missing "filter" table for ipv4 for example which is something we have previously considered mandatory.
We will still get a failure later when the network is started though I guess.
I know, and to me that's acceptable. It will not be any worse with this patch. Only better. Because right now we fail even for IPv6 even though you might not use it.
As mentioned yesterday on IRC, I hit the issue this patch tries to address on my machine. Because of $reasons, I have disabled IPv6 by adding "ipv6.disable=1" to the kernel command line (as suggested in [1]), and when running v5.1.0 or current libvirt master the default network can't be started: $ virsh net-start default error: Failed to start network default error: COMMAND_FAILED: '/usr/sbin/iptables -w10 -w --table filter --insert LIBVIRT_INP --in-interface virbr0 --protocol tcp --destination-port 67 --jump ACCEPT' failed: iptables: No chain/target/match by that name. After applying this patch, the default network comes up and works just fine. [1] https://wiki.archlinux.org/index.php/IPv6#Disable_IPv6 -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik