Re: [iptables RFC 2/2] libxtables: Boost rule target checks by announcing chain names

[Phil Sutter <phil@nwl.cc>]

On Thu, Mar 10, 2022 at 01:21:57PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > This is kind of a double-edged blade: the obvious downside is that
> > *tables-restore won't detect user-defined chain name and extension
> > clashes anymore. The upside is a tremendous performance improvement
> > restoring large rulesets. The same crooked ruleset as mentioned in
> > earlier patches (50k chains, 130k rules of which 90k jump to a chain)
> > yields these numbers:
> > 
> > variant	 unoptimized	non-targets cache	announced chains
> > ----------------------------------------------------------------
> > legacy   1m12s		37s			2.5s
> > nft      1m35s		53s			8s
> 
> I think the benefits outweight the possible issues.
> 
> > Note that iptables-legacy-restore allows the clashes already as long as
> > the name does not match a standard target, but with this patch it stops
> > warning about it.
> 
> Hmm.  That seems fixable by refusing the announce in the clash case?

When parsing a chain line, iptables-restore does not know there is a
clash because this series effectively disables that check. Due to the
non-targets hash, any chain name is looked up (as target) only once
anyway, so keeping that check in iptables-restore yields the same
performance as without the annotate.

> > iptables-nft-restore does not care at all, even allows
> > adding a chain named 'ACCEPT' (and rules can't reach it because '-j
> > ACCEPT' translates to a native nftables verdict). The latter is a bug by
> > itself.
> 
> Agree, thats a bug, it should not allow users to do that.

ACK, I'll find a fix. In legacy, libiptc (TC_CREATE_CHAIN) does it, so
an nft-specific one it will be.

Thanks, Phil