[libvirt] [PATCH] iptablesSetupPrivateChains: Be forgiving if a table does not exist

[Daniel P. Berrangé]

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 :|