src/util/viriptables.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.