Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Phil Sutter <phil@nwl.cc>,
	Arturo Borrero Gonzalez <arturo@netfilter.org>,
	netfilter-devel@vger.kernel.org
Subject: Re: [iptables PATCH] iptables-nft: fix basechain policy configuration
Date: Fri, 2 Oct 2020 16:39:26 +0200	[thread overview]
Message-ID: <20201002143926.GA1254@salvia> (raw)
In-Reply-To: <20201002133127.GD29050@orbyte.nwl.cc>

On Fri, Oct 02, 2020 at 03:31:27PM +0200, Phil Sutter wrote:
> On Fri, Oct 02, 2020 at 02:47:41PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Oct 02, 2020 at 02:28:52PM +0200, Phil Sutter wrote:
> > > On Fri, Oct 02, 2020 at 02:15:58PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > > > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > > > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > > > > trying to send an invalid ruleset to the kernel.
> > > > > 
> > > > > I fear this is not sufficient: iptables-legacy-restore leaves the
> > > > > previous chain policy in place if '-' is given in dump file. Please try
> > > > > this snippet from a testcase I wrote:
> > > > > 
> > > > > $XT_MULTI iptables -P FORWARD DROP
> > > > > 
> > > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > > >            <(echo ":FORWARD DROP [0:0]")
> > > > > 
> > > > > $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> > > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > > >            <(echo ":FORWARD DROP [0:0]")
> > > > 
> > > > Hm, this is how it works in this patch right?
> > > > 
> > > > I mean, if '-' is given, chain policy attribute in the netlink message
> > > > is not set, and the kernel sets chain policy to
> > > > NFT_CHAIN_POLICY_UNSET.
> > > > 
> > > > Or am I missing anything?
> > > 
> > > This is *flushing* iptables-restore. We're dropping the chain first and
> > > then reinstall it.
> > 
> > OK, so this fix only works with --noflush.
> > 
> > If --noflush is not specified, then it should be possible to extend
> > the cache to dump the chains, get the existing policy and use it.
> 
> nft_cmd_chain_restore() already sets NFT_CL_CHAINS.
> 
> > There is now a phase to evaluate the cache requirements, so you can
> > fetch this. Then, from the netlink phase, look up for the existing
> > policy in the cache and use it.
> 
> After the cache is fetched, nft_table_flush() runs before
> nft_chain_restore() does.
> 
> > > Another quirk is that iptables-legacy-restore ignores the counters if
> > > policy is '-' even if --counters flag was given. (:
> > 
> > OK, so this needs two more fixed on top of this one.
> 
> In Arturo's mail, he doesn't use --noflush. Not sure if this is just his
> reproducer or if OpenStack doesn't use --noflush, either. If so, your
> fix won't help with his problem.
>
> Arturo, does fixing --noflush suffice for your case? If so, we could
> delay the "--flush" case "for later". ;)

I would expect OpenStack uses --noflush. I'm not sure I can find a
use-case for the "--flush" case with policy '-'.

Let's wait for Arturo's feedback.