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

[Phil Sutter <phil@nwl.cc>]

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". ;)

Cheers, Phil