Re: [iptables PATCH v2 08/24] nft: Fetch only chains in nft_chain_list_get()

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Sep 25, 2019 at 11:25:49PM +0200, Phil Sutter wrote:
> The function is used to return the given table's chains, so fetching
> chain cache is enough.
> 
> This requires a bunch of manual rule cache updates in different places.
> To still support the fake cache logic from xtables-restore, make
> fetch_rule_cache() do nothing in case have_cache is set.
> 
> Accidental double rule cache updates for the same chain need to be
> prevented. This is complicated by the fact that chain's rule list is
> managed by libnftnl. Hence the same logic as used for table list, namely
> checking list pointer value, can't be used. Instead, simply fetch rules
> only if the given chain's rule list is empty. If it isn't, rules have
> been fetched before; if it is, a second rule fetch won't hurt.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 7c974af8b4141..729b88d990f9f 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -1264,6 +1264,7 @@ err:
>  
>  static struct nftnl_chain *
>  nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
> +static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c);
>  
>  int
>  nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> @@ -1275,6 +1276,14 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
>  
>  	nft_xt_builtin_init(h, table);
>  
> +	/* Since ebtables user-defined chain policies are implemented as last
> +	 * rule in nftables, rule cache is required here to treat them right. */
> +	if (h->family == NFPROTO_BRIDGE) {
> +		c = nft_chain_find(h, table, chain);
> +		if (c && !nft_chain_builtin(c))
> +			fetch_rule_cache(h, c);
> +	}
> +
>  	nft_fn = nft_rule_append;
>  
>  	r = nft_rule_new(h, chain, table, data);
> @@ -1550,6 +1559,9 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
>  	struct nftnl_rule *rule;
>  	int ret;
>  
> +	if (nftnl_rule_lookup_byindex(c, 0))
> +		return 0;
> +
>  	rule = nftnl_rule_alloc();
>  	if (!rule)
>  		return -1;
> @@ -1579,6 +1591,9 @@ static int fetch_rule_cache(struct nft_handle *h, struct nftnl_chain *c)
>  {
>  	int i;
>  
> +	if (h->have_cache)
> +		return 0;
> +
>  	if (c)
>  		return nft_rule_list_update(c, h);
>  
> @@ -1670,7 +1685,8 @@ struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
>  	if (!t)
>  		return NULL;
>  
> -	nft_build_cache(h);
> +	if (!h->have_cache && !h->cache->table[t->type].chains)
> +		fetch_chain_cache(h);

Could we extend nft_build_cache(...) to be used from everywhere in
this code?

Or add something like:

        nft_build_table_cache(...)
        nft_build_chain_cache(...)
        nft_build_rule_cache(...)

that actually call __nft_build_cache(...) with many parameters to
specify what table/chain/... specifically you need.

I don't have any specific design in mind for this API. However, I
would like to see a single routine to build a cache the way you need.
That single routine will ensure cache consistency, no matter what
configuration of partial cache you need.

While speeding up things, cache consistency needs to be guaranteed.