[iptables,v2,03/14] xtables: Implement per chain rule cache

Use recently introduced support for rules inside chains in libnftnl to
introduce a rule cache per chain instead of a global one.

A tricky bit is to decide if cache should be updated or not. Previously,
the global rule cache was populated just once and then reused unless
being flushed completely (via call to flush_rule_cache() with
NULL-pointer table argument). Resemble this behaviour by introducing a
boolean indicating cache status and fetch rules for all chains when
updating the chain cache in nft_chain_list_get().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Update rule cache only if required.
---
 iptables/nft.c             | 599 +++++++++++++++++--------------------
 iptables/nft.h             |   5 +-
 iptables/xtables-restore.c |   5 +-
 iptables/xtables-save.c    |   6 +-
 4 files changed, 282 insertions(+), 333 deletions(-)

Hi Phil,

On Thu, Dec 13, 2018 at 12:15:56PM +0100, Phil Sutter wrote:
> Use recently introduced support for rules inside chains in libnftnl to
> introduce a rule cache per chain instead of a global one.
> 
> A tricky bit is to decide if cache should be updated or not. Previously,
> the global rule cache was populated just once and then reused unless
> being flushed completely (via call to flush_rule_cache() with
> NULL-pointer table argument). Resemble this behaviour by introducing a
> boolean indicating cache status and fetch rules for all chains when
> updating the chain cache in nft_chain_list_get().

This is a rather large rewrite: Nice because it's going to speed up
things.

I think we should start doing ruleset generation tracking, IIRC this
is missing in the existing codebase. Such approach requires to keep
track of the ruleset generation number and invalidate caches.

Let me do a bit of reviewing before we get this in, see below. Thanks!

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Update rule cache only if required.
> ---
>  iptables/nft.c             | 599 +++++++++++++++++--------------------
>  iptables/nft.h             |   5 +-
>  iptables/xtables-restore.c |   5 +-
>  iptables/xtables-save.c    |   6 +-
>  4 files changed, 282 insertions(+), 333 deletions(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index e7a56778f8004..0869f2e222393 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
[...]
> @@ -687,10 +687,11 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
>  static void nft_chain_builtin_init(struct nft_handle *h,
>  				   const struct builtin_table *table)
>  {
> -	struct nftnl_chain_list *list = nft_chain_list_get(h, table->name);
> +	struct nftnl_chain_list *list;
>  	struct nftnl_chain *c;
>  	int i;
>  
> +	list = nft_chain_list_get(h, table->name, false);
>  	if (!list)
>  		return;
>  
> @@ -772,28 +773,15 @@ int nft_init(struct nft_handle *h, const struct builtin_table *t)
>  
>  static int __flush_rule_cache(struct nftnl_rule *r, void *data)
>  {
> -	const char *tablename = data;
> -
> -	if (!strcmp(nftnl_rule_get_str(r, NFTNL_RULE_TABLE), tablename)) {
> -		nftnl_rule_list_del(r);
> -		nftnl_rule_free(r);
> -	}
> +	nftnl_rule_list_del(r);
> +	nftnl_rule_free(r);
>  
>  	return 0;
>  }
>  
> -static void flush_rule_cache(struct nft_handle *h, const char *tablename)
> +static void flush_rule_cache(struct nftnl_chain *c)
>  {
> -	if (!h->rule_cache)
> -		return;
> -
> -	if (tablename) {
> -		nftnl_rule_list_foreach(h->rule_cache, __flush_rule_cache,
> -					(void *)tablename);
> -	} else {
> -		nftnl_rule_list_free(h->rule_cache);
> -		h->rule_cache = NULL;
> -	}
> +	nftnl_rule_foreach(c, __flush_rule_cache, NULL);
>  }
>  
>  static int __flush_chain_cache(struct nftnl_chain *c, void *data)
> @@ -815,23 +803,27 @@ static void flush_chain_cache(struct nft_handle *h, const char *tablename)
>  		if (tablename && strcmp(h->tables[i].name, tablename))
>  			continue;
>  
> -		if (h->table[i].chain_cache) {
> -			if (tablename) {
> -				nftnl_chain_list_foreach(h->table[i].chain_cache,
> -							 __flush_chain_cache, NULL);
> -				break;
> -			} else {
> -				nftnl_chain_list_free(h->table[i].chain_cache);
> -				h->table[i].chain_cache = NULL;
> -			}
> +		if (!h->table[i].chain_cache) {
> +			if (tablename)
> +				return;
> +			continue;
>  		}
> +		if (tablename) {
> +			nftnl_chain_list_foreach(h->table[i].chain_cache,
> +						 __flush_chain_cache, NULL);
> +			return;
> +		}
> +
> +		nftnl_chain_list_free(h->table[i].chain_cache);
> +		h->table[i].chain_cache = NULL;
>  	}
> +	h->have_cache = false;

Maybe

        h->have_chain_cache

instead?

> +	h->have_rule_cache = false;
>  }
>  
>  void nft_fini(struct nft_handle *h)
>  {
>  	flush_chain_cache(h, NULL);
> -	flush_rule_cache(h, NULL);
>  	mnl_socket_close(h->nl);
>  }
>  
> @@ -1191,12 +1183,15 @@ err:
>  	return NULL;
>  }
>  
> -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h);

Can you leave this function around?

Idea is that we call this function if we want a full cache, ie. chain + rules.

Then, we call nft_chain_list_get() to fetch chains only.

Otherwise, nft_rule_cache_get() ? Just trying this patch smaller here
in this round for easier review.

> +static struct nftnl_chain *
> +nft_chain_find(struct nft_handle *h, const char *table,
> +	       const char *chain, bool need_rules);

We can probably use:

        nft_chain_find()

if we want a full cache, ie. give me chain and retrieve rules
(populate cache if needed).

Otherwise, use:

        __nft_chain_find()

if we want only to fetch the chain cache, so we don't need the "bool
need_rules". Only two spots use nft_chain_find(..., false) in this
patch that I can see.

>  int
>  nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
>  		void *data, uint64_t handle, bool verbose)
>  {
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
>  	int type;
>  
> @@ -1224,10 +1219,9 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
>  	if (verbose)
>  		h->ops->print_rule(r, 0, FMT_PRINT_RULE);
>  
> -	if (!nft_rule_list_get(h))
> -		return 0;
> -
> -	nftnl_rule_list_add_tail(r, h->rule_cache);
> +	c = nft_chain_find(h, table, chain, true);

I guess we always find a match from nft_chain_find().

> +	if (c)

So this branch is probably not needed, otherwise do no ignore this
error?

> +		nftnl_chain_rule_add(r, c);
>  
>  	return 1;
>  }
> @@ -1282,12 +1276,6 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
>  	if (!t)
>  		goto out;
>  
> -	if (!h->table[t->type].chain_cache) {
> -		h->table[t->type].chain_cache = nftnl_chain_list_alloc();
> -		if (!h->table[t->type].chain_cache)
> -			goto out;
> -	}
> -
>  	nftnl_chain_list_add_tail(c, h->table[t->type].chain_cache);
>  
>  	return MNL_CB_OK;
> @@ -1297,33 +1285,60 @@ err:
>  	return MNL_CB_OK;
>  }
>  
> +static int nft_rule_list_update(struct nftnl_chain *c, void *data);
> +
>  struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
> -					    const char *table)
> +					    const char *table, bool need_rules)
>  {
>  	char buf[16536];
>  	struct nlmsghdr *nlh;
>  	const struct builtin_table *t;
>  	int ret;
> +	int i;
>  
>  	t = nft_table_builtin_find(h, table);
>  	if (!t)
>  		return NULL;

This chunk below...

> -	if (h->table[t->type].chain_cache)
> +	if (h->have_cache && (!need_rules || h->have_rule_cache))
>  		return h->table[t->type].chain_cache;
> +
> +	if (h->have_cache)
> +		goto fetch_rules;

probably code above can be rework to this below?

        if (h->have_cache) {
                if (!need_rules || h->have_rule_cache)
         		return h->table[t->type].chain_cache;

		goto fetch_rules;
        }

We can probably remove "goto fetch_rules;" and make this a function.

        if (h->have_cache) {
                if (!need_rules || h->have_rule_cache)
         		return h->table[t->type].chain_cache;

                fetch_rules_only();
        } else {
                fetch_full();
        }

so we use a bit less of goto for non-error path.

> +
>  retry:
> +	for (i = 0; i < NFT_TABLE_MAX; i++) {
> +		enum nft_table_type type = h->tables[i].type;
> +
> +		if (!h->tables[i].name)
> +			continue;
> +
> +		h->table[type].chain_cache = nftnl_chain_list_alloc();
> +	}
>  	nlh = nftnl_chain_nlmsg_build_hdr(buf, NFT_MSG_GETCHAIN, h->family,
>  					NLM_F_DUMP, h->seq);
>  
>  	ret = mnl_talk(h, nlh, nftnl_chain_list_cb, h);
>  	if (ret < 0 && errno == EINTR) {
>  		assert(nft_restart(h) >= 0);
> +		flush_chain_cache(h, NULL);
>  		goto retry;
>  	}
> +	h->have_cache = true;
> +	if (!need_rules)
> +		return h->table[t->type].chain_cache;
>  
> -	if (!h->table[t->type].chain_cache)
> -		h->table[t->type].chain_cache = nftnl_chain_list_alloc();
> +fetch_rules:
> +	for (i = 0; i < NFT_TABLE_MAX; i++) {
> +		enum nft_table_type type = h->tables[i].type;
> +
> +		if (!h->tables[i].name)
> +			continue;
>  
> +		nftnl_chain_list_foreach(h->table[type].chain_cache,
> +					 nft_rule_list_update, h);
> +	}
> +	h->have_rule_cache = true;
>  	return h->table[t->type].chain_cache;
>  }
>  
> @@ -1369,92 +1384,108 @@ int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list)
>  
>  static int nftnl_rule_list_cb(const struct nlmsghdr *nlh, void *data)
>  {
> +	struct nftnl_chain *c = data;
>  	struct nftnl_rule *r;
> -	struct nftnl_rule_list *list = data;
>  
>  	r = nftnl_rule_alloc();
>  	if (r == NULL)
> -		goto err;
> -
> -	if (nftnl_rule_nlmsg_parse(nlh, r) < 0)
> -		goto out;
> +		return MNL_CB_OK;
>  
> -	nftnl_rule_list_add_tail(r, list);
> +	if (nftnl_rule_nlmsg_parse(nlh, r) < 0) {
> +		nftnl_rule_free(r);
> +		return MNL_CB_OK;
> +	}
>  
> -	return MNL_CB_OK;
> -out:
> -	nftnl_rule_free(r);
> -	nftnl_rule_list_free(list);
> -err:
> +	nftnl_chain_rule_add(r, c);
>  	return MNL_CB_OK;
>  }
>  
> -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h)
> +static int nft_rule_list_update(struct nftnl_chain *c, void *data)

This nft_rule_list_update() looks like our former nft_rule_list_get().

>  {
> +	struct nft_handle *h = data;
>  	char buf[16536];
>  	struct nlmsghdr *nlh;
> -	struct nftnl_rule_list *list;
> +	struct nftnl_rule *rule;
>  	int ret;
>  
> -	if (h->rule_cache)
> -		return h->rule_cache;
> +	rule = nftnl_rule_alloc();
> +	if (!rule)
> +		return false;
>  
> -retry:
> -	list = nftnl_rule_list_alloc();
> -	if (list == NULL)
> -		return 0;
> +	nftnl_rule_set(rule, NFTNL_RULE_TABLE,
> +		       nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));

Use nftnl_rule_set_str() instead. I know this code it probably using
this old interface (long time ago there was not nftnl_rule_set_str()).

> +	nftnl_rule_set(rule, NFTNL_RULE_CHAIN,
> +		       nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
>  
> +retry:
>  	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, h->family,
>  					NLM_F_DUMP, h->seq);
> +	nftnl_rule_nlmsg_build_payload(nlh, rule);
>  
> -	ret = mnl_talk(h, nlh, nftnl_rule_list_cb, list);
> +	ret = mnl_talk(h, nlh, nftnl_rule_list_cb, c);
>  	if (ret < 0) {
> +		flush_rule_cache(c);
> +
>  		if (errno == EINTR) {
>  			assert(nft_restart(h) >= 0);
> -			nftnl_rule_list_free(list);
>  			goto retry;
>  		}
> -
> -		nftnl_rule_list_free(list);
> -		return NULL;
> +		nftnl_rule_free(rule);
> +		return false;
>  	}
>  
> -	h->rule_cache = list;
> -	return list;
> +	nftnl_rule_free(rule);
> +	return true;
>  }
>  
> -int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
> +static int nft_chain_save_rules(struct nft_handle *h,
> +				struct nftnl_chain *c, unsigned int format)
>  {
> -	struct nftnl_rule_list *list;
> -	struct nftnl_rule_list_iter *iter;
> +	struct nftnl_rule_iter *iter;
>  	struct nftnl_rule *r;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> -		return 0;
> -
> -	iter = nftnl_rule_list_iter_create(list);
> +	iter = nftnl_rule_iter_create(c);
>  	if (iter == NULL)
> -		return 0;
> +		return 1;

Probably we can start using -1 for errors and 0 for success
incrementally in this code? So we don't stick to the old/original
iptables function error codes anymore from nft.c.

In this case, nft_chain_save_rules() returns -1 for error.

> -	r = nftnl_rule_list_iter_next(iter);
> +	r = nftnl_rule_iter_next(iter);
>  	while (r != NULL) {
> -		const char *rule_table =
> -			nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
> +		nft_rule_print_save(r, NFT_RULE_APPEND, format);
> +		r = nftnl_rule_iter_next(iter);
> +	}
>  
> -		if (strcmp(table, rule_table) != 0)
> -			goto next;
> +	nftnl_rule_iter_destroy(iter);
> +	return 0;
> +}
>  
> -		nft_rule_print_save(r, NFT_RULE_APPEND, format);
> +int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
> +{
> +	struct nftnl_chain_list_iter *iter;
> +	struct nftnl_chain_list *list;
> +	struct nftnl_chain *c;
> +	int ret = 0;
>  
> -next:
> -		r = nftnl_rule_list_iter_next(iter);
> +	list = nft_chain_list_get(h, table, true);
> +	if (!list)
> +		return 0;
> +
> +	iter = nftnl_chain_list_iter_create(list);
> +	if (!iter)
> +		return 0;
> +
> +	c = nftnl_chain_list_iter_next(iter);
> +	while (c) {
> +		ret = nft_chain_save_rules(h, c, format);
> +		if (ret != 0)
> +			break;
> +
> +		c = nftnl_chain_list_iter_next(iter);
>  	}
>  
> -	nftnl_rule_list_iter_destroy(iter);
> +	nftnl_chain_list_iter_destroy(iter);
>  
>  	/* the core expects 1 for success and 0 for error */
> -	return 1;
> +	return ret == 0 ? 1 : 0;
>  }
>  
>  static void
> @@ -1529,7 +1560,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
>  
>  	nft_fn = nft_rule_flush;
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, false);
>  	if (list == NULL) {
>  		ret = 1;
>  		goto err;
> @@ -1553,6 +1584,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
>  			fprintf(stdout, "Flushing chain `%s'\n", chain_name);
>  
>  		__nft_rule_flush(h, table, chain_name);
> +		flush_rule_cache(c);

Place this flush_rule_cache() call in __nft_rule_flush() ?

>  
>  		if (chain != NULL)
>  			break;
> @@ -1560,7 +1592,6 @@ next:
>  		c = nftnl_chain_list_iter_next(iter);
>  	}
>  	nftnl_chain_list_iter_destroy(iter);
> -	flush_rule_cache(h, table);
>  err:
>  	/* the core expects 1 for success and 0 for error */
>  	return ret == 0 ? 1 : 0;
> @@ -1587,7 +1618,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
>  
>  	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, false);
>  	if (list)
>  		nftnl_chain_list_add(c, list);
>  
> @@ -1611,7 +1642,7 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
>  
>  	nft_fn = nft_chain_user_del;
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, false);
>  	if (list == NULL)
>  		goto err;
>  
> @@ -1689,11 +1720,12 @@ next:
>  }
>  
>  static struct nftnl_chain *
> -nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
> +nft_chain_find(struct nft_handle *h, const char *table,
> +	       const char *chain, bool need_rules)
>  {
>  	struct nftnl_chain_list *list;
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, need_rules);
>  	if (list == NULL)
>  		return NULL;
>  
> @@ -1712,7 +1744,7 @@ bool nft_chain_exists(struct nft_handle *h,
>  	if (nft_chain_builtin_find(t, chain))
>  		return true;
>  
> -	return !!nft_chain_find(h, table, chain);
> +	return !!nft_chain_find(h, table, chain, false);
>  }
>  
>  int nft_chain_user_rename(struct nft_handle *h,const char *chain,
> @@ -1732,7 +1764,7 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
>  	errno = 0;
>  
>  	/* Find the old chain to be renamed */
> -	c = nft_chain_find(h, table, chain);
> +	c = nft_chain_find(h, table, chain, false);
>  	if (c == NULL) {
>  		errno = ENOENT;
>  		return 0;
> @@ -1884,7 +1916,6 @@ static int __nft_table_flush(struct nft_handle *h, const char *table)
>  	h->table[_t->type].initialized = false;
>  
>  	flush_chain_cache(h, table);
> -	flush_rule_cache(h, table);
>  
>  	return 0;
>  }
> @@ -1925,12 +1956,6 @@ next:
>  		t = nftnl_table_list_iter_next(iter);
>  	}
>  
> -	if (!h->rule_cache) {
> -		h->rule_cache = nftnl_rule_list_alloc();
> -		if (h->rule_cache == NULL)
> -			return -1;
> -	}
> -
>  err_table_iter:
>  	nftnl_table_list_iter_destroy(iter);
>  err_table_list:
> @@ -1946,8 +1971,7 @@ void nft_table_new(struct nft_handle *h, const char *table)
>  		nft_xt_builtin_init(h, table);
>  }
>  
> -static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule_list *list,
> -			  struct nftnl_rule *r)
> +static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule *r)
>  {
>  	int ret;
>  
> @@ -1962,31 +1986,19 @@ static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule_list *list,
>  }
>  
>  static struct nftnl_rule *
> -nft_rule_find(struct nft_handle *h, struct nftnl_rule_list *list,
> -	      const char *chain, const char *table, void *data, int rulenum)
> +nft_rule_find(struct nft_handle *h, struct nftnl_chain *c, void *data, int rulenum)
>  {
>  	struct nftnl_rule *r;
> -	struct nftnl_rule_list_iter *iter;
> +	struct nftnl_rule_iter *iter;
>  	int rule_ctr = 0;
>  	bool found = false;
>  
> -	iter = nftnl_rule_list_iter_create(list);
> +	iter = nftnl_rule_iter_create(c);
>  	if (iter == NULL)
>  		return 0;
>  
> -	r = nftnl_rule_list_iter_next(iter);
> +	r = nftnl_rule_iter_next(iter);
>  	while (r != NULL) {
> -		const char *rule_table =
> -			nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
> -		const char *rule_chain =
> -			nftnl_rule_get_str(r, NFTNL_RULE_CHAIN);
> -
> -		if (strcmp(table, rule_table) != 0 ||
> -		    strcmp(chain, rule_chain) != 0) {
> -			DEBUGP("different chain / table\n");
> -			goto next;
> -		}
> -
>  		if (rulenum >= 0) {
>  			/* Delete by rule number case */
>  			if (rule_ctr == rulenum) {
> @@ -1999,11 +2011,10 @@ nft_rule_find(struct nft_handle *h, struct nftnl_rule_list *list,
>  				break;
>  		}
>  		rule_ctr++;
> -next:
> -		r = nftnl_rule_list_iter_next(iter);
> +		r = nftnl_rule_iter_next(iter);
>  	}
>  
> -	nftnl_rule_list_iter_destroy(iter);
> +	nftnl_rule_iter_destroy(iter);
>  
>  	return found ? r : NULL;
>  }
> @@ -2011,16 +2022,16 @@ next:
>  int nft_rule_check(struct nft_handle *h, const char *chain,
>  		   const char *table, void *data, bool verbose)
>  {
> -	struct nftnl_rule_list *list;
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
>  
>  	nft_fn = nft_rule_check;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c)
>  		return 0;
>  
> -	r = nft_rule_find(h, list, chain, table, data, -1);
> +	r = nft_rule_find(h, c, data, -1);
>  	if (r == NULL) {
>  		errno = ENOENT;
>  		return 0;
> @@ -2034,19 +2045,21 @@ int nft_rule_check(struct nft_handle *h, const char *chain,
>  int nft_rule_delete(struct nft_handle *h, const char *chain,
>  		    const char *table, void *data, bool verbose)
>  {
> -	int ret = 0;
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
> -	struct nftnl_rule_list *list;
> +	int ret = 0;
>  
>  	nft_fn = nft_rule_delete;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c) {
> +		errno = ENOENT;
>  		return 0;
> +	}
>  
> -	r = nft_rule_find(h, list, chain, table, data, -1);
> +	r = nft_rule_find(h, c, data, -1);
>  	if (r != NULL) {
> -		ret =__nft_rule_del(h, list, r);
> +		ret =__nft_rule_del(h, r);
>  		if (ret < 0)
>  			errno = ENOMEM;
>  		if (verbose)
> @@ -2086,7 +2099,7 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
>  		    const char *table, void *data, int rulenum, bool verbose)
>  {
>  	struct nftnl_rule *r, *new_rule;
> -	struct nftnl_rule_list *list;
> +	struct nftnl_chain *c;
>  	uint64_t handle = 0;
>  
>  	/* If built-in chains don't exist for this table, create them */
> @@ -2095,18 +2108,19 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
>  
>  	nft_fn = nft_rule_insert;
>  
> -	if (rulenum > 0) {
> -		list = nft_rule_list_get(h);
> -		if (list == NULL)
> -			goto err;
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c) {
> +		errno = ENOENT;
> +		goto err;
> +	}
>  
> -		r = nft_rule_find(h, list, chain, table, data, rulenum);
> +	if (rulenum > 0) {
> +		r = nft_rule_find(h, c, data, rulenum);
>  		if (r == NULL) {
>  			/* special case: iptables allows to insert into
>  			 * rule_count + 1 position.
>  			 */
> -			r = nft_rule_find(h, list, chain, table, data,
> -					  rulenum - 1);
> +			r = nft_rule_find(h, c, data, rulenum - 1);
>  			if (r != NULL)
>  				return nft_rule_append(h, chain, table, data,
>  						       0, verbose);
> @@ -2117,8 +2131,6 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
>  
>  		handle = nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE);
>  		DEBUGP("adding after rule handle %"PRIu64"\n", handle);
> -	} else {
> -		nft_rule_list_get(h);
>  	}
>  
>  	new_rule = nft_rule_add(h, chain, table, data, handle, verbose);
> @@ -2126,9 +2138,9 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
>  		goto err;
>  
>  	if (handle)
> -		nftnl_rule_list_insert_at(new_rule, r);
> +		nftnl_chain_rule_insert_at(new_rule, r);
>  	else
> -		nftnl_rule_list_add(new_rule, h->rule_cache);
> +		nftnl_chain_rule_add(new_rule, c);
>  
>  	return 1;
>  err:
> @@ -2138,20 +2150,22 @@ err:
>  int nft_rule_delete_num(struct nft_handle *h, const char *chain,
>  			const char *table, int rulenum, bool verbose)
>  {
> -	int ret = 0;
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
> -	struct nftnl_rule_list *list;
> +	int ret = 0;
>  
>  	nft_fn = nft_rule_delete_num;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c) {
> +		errno = ENOENT;
>  		return 0;
> +	}
>  
> -	r = nft_rule_find(h, list, chain, table, NULL, rulenum);
> +	r = nft_rule_find(h, c, NULL, rulenum);
>  	if (r != NULL) {
>  		DEBUGP("deleting rule by number %d\n", rulenum);
> -		ret = __nft_rule_del(h, list, r);
> +		ret = __nft_rule_del(h, r);
>  		if (ret < 0)
>  			errno = ENOMEM;
>  	} else
> @@ -2163,17 +2177,19 @@ int nft_rule_delete_num(struct nft_handle *h, const char *chain,
>  int nft_rule_replace(struct nft_handle *h, const char *chain,
>  		     const char *table, void *data, int rulenum, bool verbose)
>  {
> -	int ret = 0;
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
> -	struct nftnl_rule_list *list;
> +	int ret = 0;
>  
>  	nft_fn = nft_rule_replace;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c) {
> +		errno = ENOENT;
>  		return 0;
> +	}
>  
> -	r = nft_rule_find(h, list, chain, table, data, rulenum);
> +	r = nft_rule_find(h, c, data, rulenum);
>  	if (r != NULL) {
>  		DEBUGP("replacing rule with handle=%llu\n",
>  			(unsigned long long)
> @@ -2191,35 +2207,21 @@ int nft_rule_replace(struct nft_handle *h, const char *chain,
>  }
>  
>  static int
> -__nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
> +__nft_rule_list(struct nft_handle *h, struct nftnl_chain *c,
>  		int rulenum, unsigned int format,
>  		void (*cb)(struct nftnl_rule *r, unsigned int num,
>  			   unsigned int format))
>  {
> -	struct nftnl_rule_list *list;
> -	struct nftnl_rule_list_iter *iter;
> +	struct nftnl_rule_iter *iter;
>  	struct nftnl_rule *r;
>  	int rule_ctr = 0;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> -		return 0;
> -
> -	iter = nftnl_rule_list_iter_create(list);
> +	iter = nftnl_rule_iter_create(c);
>  	if (iter == NULL)
>  		return 0;
>  
> -	r = nftnl_rule_list_iter_next(iter);
> +	r = nftnl_rule_iter_next(iter);
>  	while (r != NULL) {
> -		const char *rule_table =
> -			nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
> -		const char *rule_chain =
> -			nftnl_rule_get_str(r, NFTNL_RULE_CHAIN);
> -
> -		if (strcmp(table, rule_table) != 0 ||
> -		    strcmp(chain, rule_chain) != 0)
> -			goto next;
> -
>  		rule_ctr++;
>  
>  		if (rulenum > 0 && rule_ctr != rulenum) {
> @@ -2232,46 +2234,30 @@ __nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
>  			break;
>  
>  next:
> -		r = nftnl_rule_list_iter_next(iter);
> +		r = nftnl_rule_iter_next(iter);
>  	}
>  
> -	nftnl_rule_list_iter_destroy(iter);
> +	nftnl_rule_iter_destroy(iter);
>  	return 1;
>  }
>  
> -static int nft_rule_count(struct nft_handle *h,
> -			  const char *chain, const char *table)
> +static int nft_rule_count(struct nft_handle *h, struct nftnl_chain *c)
>  {
> -	struct nftnl_rule_list_iter *iter;
> -	struct nftnl_rule_list *list;
> +	struct nftnl_rule_iter *iter;
>  	struct nftnl_rule *r;
>  	int rule_ctr = 0;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> -		return 0;
> -
> -	iter = nftnl_rule_list_iter_create(list);
> +	iter = nftnl_rule_iter_create(c);
>  	if (iter == NULL)
>  		return 0;
>  
> -	r = nftnl_rule_list_iter_next(iter);
> +	r = nftnl_rule_iter_next(iter);
>  	while (r != NULL) {
> -		const char *rule_table =
> -			nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
> -		const char *rule_chain =
> -			nftnl_rule_get_str(r, NFTNL_RULE_CHAIN);
> -
> -		if (strcmp(table, rule_table) != 0 ||
> -		    strcmp(chain, rule_chain) != 0)
> -			goto next;
> -
>  		rule_ctr++;
> -next:
> -		r = nftnl_rule_list_iter_next(iter);
> +		r = nftnl_rule_iter_next(iter);
>  	}
>  
> -	nftnl_rule_list_iter_destroy(iter);
> +	nftnl_rule_iter_destroy(iter);
>  	return rule_ctr;
>  }
>  
> @@ -2304,8 +2290,11 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
>  	}
>  
>  	if (chain && rulenum) {
> -		__nft_rule_list(h, chain, table,
> -				rulenum, format, ops->print_rule);
> +		c = nft_chain_find(h, table, chain, true);
> +		if (!c)
> +			return 0;
> +
> +		__nft_rule_list(h, c, rulenum, format, ops->print_rule);
>  		return 1;
>  	}
>  
> @@ -2348,12 +2337,11 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
>  		if (found)
>  			printf("\n");
>  
> -		entries = nft_rule_count(h, chain_name, table);
> +		entries = nft_rule_count(h, c);
>  		ops->print_header(format, chain_name, policy_name[policy],
>  				  &ctrs, basechain, refs - entries, entries);
>  
> -		__nft_rule_list(h, chain_name, table,
> -				rulenum, format, ops->print_rule);
> +		__nft_rule_list(h, c, rulenum, format, ops->print_rule);
>  
>  		found = true;
>  
> @@ -2452,7 +2440,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
>  		return 0;
>  	}
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, true);
>  	if (!list)
>  		goto err;
>  
> @@ -2478,8 +2466,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
>  		if (chain && strcmp(chain, chain_name) != 0)
>  			goto next;
>  
> -		ret = __nft_rule_list(h, chain_name, table, rulenum,
> -				      format, list_save);
> +		ret = __nft_rule_list(h, c, rulenum, format, list_save);
>  
>  		/* we printed the chain we wanted, stop processing. */
>  		if (chain)
> @@ -2497,17 +2484,17 @@ int nft_rule_zero_counters(struct nft_handle *h, const char *chain,
>  			   const char *table, int rulenum)
>  {
>  	struct iptables_command_state cs = {};
> -	struct nftnl_rule_list *list;
> +	struct nftnl_chain *c;
>  	struct nftnl_rule *r;
>  	int ret = 0;
>  
>  	nft_fn = nft_rule_delete;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> +	c = nft_chain_find(h, table, chain, true);
> +	if (!c)
>  		return 0;
>  
> -	r = nft_rule_find(h, list, chain, table, NULL, rulenum);
> +	r = nft_rule_find(h, c, NULL, rulenum);
>  	if (r == NULL) {
>  		errno = ENOENT;
>  		ret = 1;
> @@ -2976,38 +2963,19 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
>  static void nft_chain_zero_rule_counters(struct nft_handle *h,
>  					 struct nftnl_chain *c)
>  {
> -	struct nftnl_rule_list_iter *iter;
> -	struct nftnl_rule_list *list;
> -	const char *table_name;
> -	const char *chain_name;
> +	struct nftnl_rule_iter *iter;
>  	struct nftnl_rule *r;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> -		return;
> -	iter = nftnl_rule_list_iter_create(list);
> +	iter = nftnl_rule_iter_create(c);
>  	if (iter == NULL)
>  		return;
>  
> -	table_name = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
> -	chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
> -
> -	r = nftnl_rule_list_iter_next(iter);
> +	r = nftnl_rule_iter_next(iter);
>  	while (r != NULL) {
>  		struct nftnl_expr_iter *ei;
> -		const char *table_chain;
> -		const char *rule_chain;
>  		struct nftnl_expr *e;
>  		bool zero_needed;
>  
> -		table_chain = nftnl_rule_get_str(r, NFTNL_RULE_TABLE);
> -		if (strcmp(table_chain, table_name))
> -			goto next;
> -
> -		rule_chain = nftnl_rule_get_str(r, NFTNL_RULE_CHAIN);
> -		if (strcmp(rule_chain, chain_name))
> -			goto next;
> -
>  		ei = nftnl_expr_iter_create(r);
>  		if (!ei)
>  			break;
> @@ -3038,11 +3006,10 @@ static void nft_chain_zero_rule_counters(struct nft_handle *h,
>  			nftnl_rule_unset(r, NFTNL_RULE_POSITION);
>  			batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r);
>  		}
> -next:
> -		r = nftnl_rule_list_iter_next(iter);
> +		r = nftnl_rule_iter_next(iter);
>  	}
>  
> -	nftnl_rule_list_iter_destroy(iter);
> +	nftnl_rule_iter_destroy(iter);
>  }
>  
>  int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
> @@ -3053,7 +3020,7 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
>  	struct nftnl_chain *c;
>  	int ret = 0;
>  
> -	list = nft_chain_list_get(h, table);
> +	list = nft_chain_list_get(h, table, true);
>  	if (list == NULL)
>  		goto err;
>  
> @@ -3119,29 +3086,29 @@ static const char *supported_exprs[NFT_COMPAT_EXPR_MAX] = {
>  };
>  
>  

This chunk below could be well in a separated patch for easier review.

> -static int nft_is_expr_compatible(const struct nftnl_expr *expr)
> +static bool nft_is_expr_compatible(const struct nftnl_expr *expr)
>  {
>  	const char *name = nftnl_expr_get_str(expr, NFTNL_EXPR_NAME);
>  	int i;
>  
>  	for (i = 0; i < NFT_COMPAT_EXPR_MAX; i++) {
>  		if (strcmp(supported_exprs[i], name) == 0)
> -			return 0;
> +			return true;
>  	}
>  
>  	if (!strcmp(name, "limit") &&
>  	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_TYPE) == NFT_LIMIT_PKTS &&
>  	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_FLAGS) == 0)
> -		return 0;
> +		return true;
>  
> -	return 1;
> +	return false;
>  }
>  
>  static bool nft_is_rule_compatible(struct nftnl_rule *rule)
>  {
>  	struct nftnl_expr_iter *iter;
>  	struct nftnl_expr *expr;
> -	bool compatible = false;
> +	bool compatible = true;

Looks like a cleanup, probably separated patch?

>  	iter = nftnl_expr_iter_create(rule);
>  	if (iter == NULL)
> @@ -3149,123 +3116,101 @@ static bool nft_is_rule_compatible(struct nftnl_rule *rule)
>  
>  	expr = nftnl_expr_iter_next(iter);
>  	while (expr != NULL) {
> -		if (nft_is_expr_compatible(expr) == 0) {
> -			expr = nftnl_expr_iter_next(iter);
> -			continue;
> +		if (!nft_is_expr_compatible(expr)) {
> +			compatible = false;
> +			break;
>  		}
> -
> -		compatible = true;
> -		break;
> +		expr = nftnl_expr_iter_next(iter);
>  	}
>  
>  	nftnl_expr_iter_destroy(iter);
>  	return compatible;
>  }
>  
> -static int nft_is_chain_compatible(const struct nft_handle *h,
> -				   const struct nftnl_chain *chain)
> +static bool nft_are_rules_compatible(struct nft_handle *h, struct nftnl_chain *c)
>  {
> -	const char *table, *name, *type, *cur_table;
> -	const struct builtin_chain *chains;
> -	int i, j, prio;
> -	enum nf_inet_hooks hook;
> -
> -	table = nftnl_chain_get(chain, NFTNL_CHAIN_TABLE);
> -	name = nftnl_chain_get(chain, NFTNL_CHAIN_NAME);
> -	type = nftnl_chain_get(chain, NFTNL_CHAIN_TYPE);
> -	prio = nftnl_chain_get_u32(chain, NFTNL_CHAIN_PRIO);
> -	hook = nftnl_chain_get_u32(chain, NFTNL_CHAIN_HOOKNUM);
> -
> -	for (i = 0; i < NFT_TABLE_MAX; i++) {
> -		cur_table = h->tables[i].name;
> -		chains = h->tables[i].chains;
> -
> -		if (!cur_table || strcmp(table, cur_table) != 0)
> -			continue;
> +	struct nftnl_rule_iter *iter;
> +	struct nftnl_rule *rule;
> +	bool compatible = true;
>  
> -		for (j = 0; j < NF_INET_NUMHOOKS && chains[j].name; j++) {
> -			if (strcmp(name, chains[j].name) != 0)
> -				continue;
> +	iter = nftnl_rule_iter_create(c);
> +	if (iter == NULL)
> +		return false;
>  
> -			if (strcmp(type, chains[j].type) == 0 &&
> -			    prio == chains[j].prio &&
> -			    hook == chains[j].hook)
> -				return 0;
> +	rule = nftnl_rule_iter_next(iter);
> +	while (rule != NULL) {
> +		if (!nft_is_rule_compatible(rule)) {
> +			compatible = false;
>  			break;
>  		}
> +		rule = nftnl_rule_iter_next(iter);
>  	}
> -
> -	return 1;
> +	nftnl_rule_iter_destroy(iter);
> +	return compatible;
>  }
>  
> -static int nft_are_chains_compatible(struct nft_handle *h, const char *tablename)
> +static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
>  {
> -	struct nftnl_chain_list *list;
> -	struct nftnl_chain_list_iter *iter;
> -	struct nftnl_chain *chain;
> -	int ret = 0;
> +	const struct builtin_chain *chains = NULL, *chain = NULL;
> +	const char *table, *name, *type;
> +	struct nft_handle *h = data;
> +	enum nf_inet_hooks hook;
> +	int i, prio;
>  
> -	list = nft_chain_list_get(h, tablename);
> -	if (list == NULL)
> +	if (!nft_are_rules_compatible(h, c))
>  		return -1;
>  
> -	iter = nftnl_chain_list_iter_create(list);
> -	if (iter == NULL)
> +	if (!nft_chain_builtin(c))
> +		return 0;
> +
> +	/* find chain's table in builtin tables */
> +	table = nftnl_chain_get(c, NFTNL_CHAIN_TABLE);
> +	for (i = 0; i < NFT_TABLE_MAX; i++) {
> +		const char *cur_table = h->tables[i].name;
> +
> +		if (!cur_table || strcmp(table, cur_table) != 0)
> +			continue;
> +
> +		chains = h->tables[i].chains;
> +		break;
> +	}
> +	if (!chains)
>  		return -1;
>  
> -	chain = nftnl_chain_list_iter_next(iter);
> -	while (chain != NULL) {
> -		if (!nft_chain_builtin(chain))
> -			goto next;
> +	/* find chain in builtin chain list */
> +	name = nftnl_chain_get(c, NFTNL_CHAIN_NAME);
> +	for (i = 0; i < NF_INET_NUMHOOKS && chains[i].name; i++) {
> +		if (strcmp(name, chains[i].name) != 0)
> +			continue;
>  
> -		ret = nft_is_chain_compatible(h, chain);
> -		if (ret != 0)
> -			break;
> -next:
> -		chain = nftnl_chain_list_iter_next(iter);
> +		chain = &chains[i];
> +		break;
>  	}
> +	if (!chain)
> +		return -1;
>  
> -	nftnl_chain_list_iter_destroy(iter);
> +	/* compare properties */
> +	type = nftnl_chain_get(c, NFTNL_CHAIN_TYPE);
> +	prio = nftnl_chain_get_u32(c, NFTNL_CHAIN_PRIO);
> +	hook = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
> +	if (strcmp(type, chains[i].type) == 0 &&
> +	    prio == chains[i].prio &&
> +	    hook == chains[i].hook)
> +		return 0;
>  
> -	return ret;
> +	return -1;
>  }
>  
>  bool nft_is_table_compatible(struct nft_handle *h, const char *tablename)
>  {
> -	struct nftnl_rule_list *list;
> -	struct nftnl_rule_list_iter *iter;
> -	struct nftnl_rule *rule;
> -	int ret = 0;
> +	struct nftnl_chain_list *list;
>  
> -	if (!nft_table_builtin_find(h, tablename))
> +	list = nft_chain_list_get(h, tablename, true);
> +	if (!list)
>  		return false;
>  
> -	ret = nft_are_chains_compatible(h, tablename);
> -	if (ret != 0)
> +	if (nftnl_chain_list_foreach(list, nft_is_chain_compatible, h))
>  		return false;
>  
> -	list = nft_rule_list_get(h);
> -	if (list == NULL)
> -		return true;
> -
> -	iter = nftnl_rule_list_iter_create(list);
> -	if (iter == NULL)
> -		return true;
> -
> -	rule = nftnl_rule_list_iter_next(iter);
> -	while (rule != NULL) {
> -		const char *table = nftnl_rule_get_str(rule, NFTNL_RULE_TABLE);
> -
> -		if (strcmp(table, tablename))
> -			goto next_rule;
> -
> -		ret = nft_is_rule_compatible(rule);
> -		if (ret != 0)
> -			break;
> -next_rule:
> -		rule = nftnl_rule_list_iter_next(iter);
> -	}
> -
> -	nftnl_rule_list_iter_destroy(iter);
> -	return ret == 0;
> +	return true;
>  }
> diff --git a/iptables/nft.h b/iptables/nft.h
> index bf60ab3943659..af46afc789773 100644
> --- a/iptables/nft.h
> +++ b/iptables/nft.h
> @@ -42,7 +42,8 @@ struct nft_handle {
>  		struct nftnl_chain_list *chain_cache;
>  		bool			initialized;
>  	} table[NFT_TABLE_MAX];
> -	struct nftnl_rule_list	*rule_cache;
> +	bool			have_cache;
> +	bool			have_rule_cache;
>  	bool			restore;
>  	int8_t			config_done;
>  
> @@ -82,7 +83,7 @@ struct nftnl_chain;
>  
>  int nft_chain_set(struct nft_handle *h, const char *table, const char *chain, const char *policy, const struct xt_counters *counters);
>  struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
> -					    const char *table);
> +					    const char *table, bool need_rules);
>  struct nftnl_chain *nft_chain_list_find(struct nftnl_chain_list *list,
>  					const char *chain);
>  int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list);
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> index 4e00ed86be06d..0aaeec0764426 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -61,7 +61,7 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
>  {
>  	struct nftnl_chain_list *chain_list;
>  
> -	chain_list = nft_chain_list_get(h, table);
> +	chain_list = nft_chain_list_get(h, table, false);
>  	if (chain_list == NULL)
>  		xtables_error(OTHER_PROBLEM, "cannot retrieve chain list\n");
>  
> @@ -155,6 +155,9 @@ void xtables_restore_parse(struct nft_handle *h,
>  					table);
>  				if (cb->table_flush)
>  					cb->table_flush(h, table);
> +				/* fake rule cache presence so that we don't populate
> +				 * the cache later with rules just flushed here */

                                /* incrementally adding netdev comment styles
                                 * would be good :-).
                                 */

> +				h->have_rule_cache = true;
>  			}
>  
>  			ret = 1;
> diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
> index 414a864b6196b..4451c9ce15557 100644
> --- a/iptables/xtables-save.c
> +++ b/iptables/xtables-save.c
> @@ -73,7 +73,7 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)
>  		return 0;
>  	}
>  
> -	chain_list = nft_chain_list_get(h, tablename);
> +	chain_list = nft_chain_list_get(h, tablename, false);
>  	if (!chain_list)
>  		return 0;
>  
> @@ -259,7 +259,7 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
>  		return 0;
>  	}
>  
> -	chain_list = nft_chain_list_get(h, tablename);
> +	chain_list = nft_chain_list_get(h, tablename, false);
>  
>  	if (first) {
>  		now = time(NULL);
> @@ -401,7 +401,7 @@ int xtables_arp_save_main(int argc, char **argv)
>  	}
>  
>  	printf("*filter\n");
> -	nft_chain_save(&h, nft_chain_list_get(&h, "filter"));
> +	nft_chain_save(&h, nft_chain_list_get(&h, "filter", false));
>  	nft_rule_save(&h, "filter", show_counters ? 0 : FMT_NOCOUNTS);
>  	printf("\n");
>  	nft_fini(&h);
> -- 
> 2.19.0
>

Hi Pablo,

On Mon, Dec 17, 2018 at 06:42:49PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 13, 2018 at 12:15:56PM +0100, Phil Sutter wrote:
> > Use recently introduced support for rules inside chains in libnftnl to
> > introduce a rule cache per chain instead of a global one.
> > 
> > A tricky bit is to decide if cache should be updated or not. Previously,
> > the global rule cache was populated just once and then reused unless
> > being flushed completely (via call to flush_rule_cache() with
> > NULL-pointer table argument). Resemble this behaviour by introducing a
> > boolean indicating cache status and fetch rules for all chains when
> > updating the chain cache in nft_chain_list_get().
> 
> This is a rather large rewrite: Nice because it's going to speed up
> things.
> 
> I think we should start doing ruleset generation tracking, IIRC this
> is missing in the existing codebase. Such approach requires to keep
> track of the ruleset generation number and invalidate caches.

Do you think parallel ruleset updates are a problem? None of the xtables
tools are meant to stay running for long, so I didn't expect problems
there until now. Sure, loading 10k rules with iptables-nft-restore takes
a long time, but if kernel's ruleset changes in between, I don't see how
we could recover from that (apart from restarting the whole restore).

> Let me do a bit of reviewing before we get this in, see below. Thanks!

Thanks for your review! Some comments below.

> 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v1:
> > - Update rule cache only if required.
> > ---
> >  iptables/nft.c             | 599 +++++++++++++++++--------------------
> >  iptables/nft.h             |   5 +-
> >  iptables/xtables-restore.c |   5 +-
> >  iptables/xtables-save.c    |   6 +-
> >  4 files changed, 282 insertions(+), 333 deletions(-)
> > 
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index e7a56778f8004..0869f2e222393 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> [...]
> > @@ -815,23 +803,27 @@ static void flush_chain_cache(struct nft_handle *h, const char *tablename)
> >  		if (tablename && strcmp(h->tables[i].name, tablename))
> >  			continue;
> >  
> > -		if (h->table[i].chain_cache) {
> > -			if (tablename) {
> > -				nftnl_chain_list_foreach(h->table[i].chain_cache,
> > -							 __flush_chain_cache, NULL);
> > -				break;
> > -			} else {
> > -				nftnl_chain_list_free(h->table[i].chain_cache);
> > -				h->table[i].chain_cache = NULL;
> > -			}
> > +		if (!h->table[i].chain_cache) {
> > +			if (tablename)
> > +				return;
> > +			continue;
> >  		}
> > +		if (tablename) {
> > +			nftnl_chain_list_foreach(h->table[i].chain_cache,
> > +						 __flush_chain_cache, NULL);
> > +			return;
> > +		}
> > +
> > +		nftnl_chain_list_free(h->table[i].chain_cache);
> > +		h->table[i].chain_cache = NULL;
> >  	}
> > +	h->have_cache = false;
> 
> Maybe
> 
>         h->have_chain_cache
> 
> instead?

ACK. Simple 'have_cache' is too generic indeed.
> 
> > +	h->have_rule_cache = false;
> >  }
> >  
> >  void nft_fini(struct nft_handle *h)
> >  {
> >  	flush_chain_cache(h, NULL);
> > -	flush_rule_cache(h, NULL);
> >  	mnl_socket_close(h->nl);
> >  }
> >  
> > @@ -1191,12 +1183,15 @@ err:
> >  	return NULL;
> >  }
> >  
> > -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h);
> 
> Can you leave this function around?
> 
> Idea is that we call this function if we want a full cache, ie. chain + rules.
> 
> Then, we call nft_chain_list_get() to fetch chains only.
> 
> Otherwise, nft_rule_cache_get() ? Just trying this patch smaller here
> in this round for easier review.

Sure, no problem.

> > +static struct nftnl_chain *
> > +nft_chain_find(struct nft_handle *h, const char *table,
> > +	       const char *chain, bool need_rules);
> 
> We can probably use:
> 
>         nft_chain_find()
> 
> if we want a full cache, ie. give me chain and retrieve rules
> (populate cache if needed).
> 
> Otherwise, use:
> 
>         __nft_chain_find()
> 
> if we want only to fetch the chain cache, so we don't need the "bool
> need_rules". Only two spots use nft_chain_find(..., false) in this
> patch that I can see.

Will do.

> >  int
> >  nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> >  		void *data, uint64_t handle, bool verbose)
> >  {
> > +	struct nftnl_chain *c;
> >  	struct nftnl_rule *r;
> >  	int type;
> >  
> > @@ -1224,10 +1219,9 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> >  	if (verbose)
> >  		h->ops->print_rule(r, 0, FMT_PRINT_RULE);
> >  
> > -	if (!nft_rule_list_get(h))
> > -		return 0;
> > -
> > -	nftnl_rule_list_add_tail(r, h->rule_cache);
> > +	c = nft_chain_find(h, table, chain, true);
> 
> I guess we always find a match from nft_chain_find().
> 
> > +	if (c)
> 
> So this branch is probably not needed, otherwise do no ignore this
> error?
> 
> > +		nftnl_chain_rule_add(r, c);

Ah, yes. In a later patch of the same series I turn this into:

|         c = nft_chain_find(h, table, chain, true);
| -       if (c)
| -               nftnl_chain_rule_add(r, c);
| +       assert(c);
| +       nftnl_chain_rule_add_tail(r, c);

Because if the rule is not added to the chain cache, it won't end in the
batch. Also the change from rule_add_tail to rule_add is a mistake here.

[...]
> > @@ -1297,33 +1285,60 @@ err:
> >  	return MNL_CB_OK;
> >  }
> >  
> > +static int nft_rule_list_update(struct nftnl_chain *c, void *data);
> > +
> >  struct nftnl_chain_list *nft_chain_list_get(struct nft_handle *h,
> > -					    const char *table)
> > +					    const char *table, bool need_rules)
> >  {
> >  	char buf[16536];
> >  	struct nlmsghdr *nlh;
> >  	const struct builtin_table *t;
> >  	int ret;
> > +	int i;
> >  
> >  	t = nft_table_builtin_find(h, table);
> >  	if (!t)
> >  		return NULL;
> 
> This chunk below...
> 
> > -	if (h->table[t->type].chain_cache)
> > +	if (h->have_cache && (!need_rules || h->have_rule_cache))
> >  		return h->table[t->type].chain_cache;
> > +
> > +	if (h->have_cache)
> > +		goto fetch_rules;
> 
> probably code above can be rework to this below?
> 
>         if (h->have_cache) {
>                 if (!need_rules || h->have_rule_cache)
>          		return h->table[t->type].chain_cache;
> 
> 		goto fetch_rules;
>         }
> 
> We can probably remove "goto fetch_rules;" and make this a function.
> 
>         if (h->have_cache) {
>                 if (!need_rules || h->have_rule_cache)
>          		return h->table[t->type].chain_cache;
> 
>                 fetch_rules_only();
>         } else {
>                 fetch_full();
>         }
> 
> so we use a bit less of goto for non-error path.

Yes, good points. I will do that.

[...]
> > -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h)
> > +static int nft_rule_list_update(struct nftnl_chain *c, void *data)
> 
> This nft_rule_list_update() looks like our former nft_rule_list_get().

Yes, I turned nft_rule_list_get() into a callback for use in
nft_chain_list_get(). The rename is more or less a side-effect of that.
I'll see if I can reuse code between nft_rule_list_get() and the
callback (if needed at all).

> >  {
> > +	struct nft_handle *h = data;
> >  	char buf[16536];
> >  	struct nlmsghdr *nlh;
> > -	struct nftnl_rule_list *list;
> > +	struct nftnl_rule *rule;
> >  	int ret;
> >  
> > -	if (h->rule_cache)
> > -		return h->rule_cache;
> > +	rule = nftnl_rule_alloc();
> > +	if (!rule)
> > +		return false;
> >  
> > -retry:
> > -	list = nftnl_rule_list_alloc();
> > -	if (list == NULL)
> > -		return 0;
> > +	nftnl_rule_set(rule, NFTNL_RULE_TABLE,
> > +		       nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
> 
> Use nftnl_rule_set_str() instead. I know this code it probably using
> this old interface (long time ago there was not nftnl_rule_set_str()).

Sure, will do.

[...]
> > -int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
> > +static int nft_chain_save_rules(struct nft_handle *h,
> > +				struct nftnl_chain *c, unsigned int format)
> >  {
> > -	struct nftnl_rule_list *list;
> > -	struct nftnl_rule_list_iter *iter;
> > +	struct nftnl_rule_iter *iter;
> >  	struct nftnl_rule *r;
> >  
> > -	list = nft_rule_list_get(h);
> > -	if (list == NULL)
> > -		return 0;
> > -
> > -	iter = nftnl_rule_list_iter_create(list);
> > +	iter = nftnl_rule_iter_create(c);
> >  	if (iter == NULL)
> > -		return 0;
> > +		return 1;
> 
> Probably we can start using -1 for errors and 0 for success
> incrementally in this code? So we don't stick to the old/original
> iptables function error codes anymore from nft.c.

Fine with me! Keeping track which function has to return what is tedious
sometimes.

[...]
> > @@ -1553,6 +1584,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
> >  			fprintf(stdout, "Flushing chain `%s'\n", chain_name);
> >  
> >  		__nft_rule_flush(h, table, chain_name);
> > +		flush_rule_cache(c);
> 
> Place this flush_rule_cache() call in __nft_rule_flush() ?

The function is used from __nft_chain_user_flush() as well where no
cache flush happens.

[...]
> This chunk below could be well in a separated patch for easier review.
> 
> > -static int nft_is_expr_compatible(const struct nftnl_expr *expr)
> > +static bool nft_is_expr_compatible(const struct nftnl_expr *expr)

Yes, you're right. I had to adjust the is_*_compatible functions so
decided to make them a bit more consistent while doing so. I'll move the
return type change into a prepended patch.

[...]
> > --- a/iptables/xtables-restore.c
> > +++ b/iptables/xtables-restore.c
[...]
> > @@ -155,6 +155,9 @@ void xtables_restore_parse(struct nft_handle *h,
> >  					table);
> >  				if (cb->table_flush)
> >  					cb->table_flush(h, table);
> > +				/* fake rule cache presence so that we don't populate
> > +				 * the cache later with rules just flushed here */
> 
>                                 /* incrementally adding netdev comment styles
>                                  * would be good :-).
>                                  */

So comment ending on new line? I always forget what's the right comment
style in each project. :)

Thanks, Phil

On Mon, Dec 17, 2018 at 07:24:24PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Mon, Dec 17, 2018 at 06:42:49PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Dec 13, 2018 at 12:15:56PM +0100, Phil Sutter wrote:
> > > Use recently introduced support for rules inside chains in libnftnl to
> > > introduce a rule cache per chain instead of a global one.
> > > 
> > > A tricky bit is to decide if cache should be updated or not. Previously,
> > > the global rule cache was populated just once and then reused unless
> > > being flushed completely (via call to flush_rule_cache() with
> > > NULL-pointer table argument). Resemble this behaviour by introducing a
> > > boolean indicating cache status and fetch rules for all chains when
> > > updating the chain cache in nft_chain_list_get().
> > 
> > This is a rather large rewrite: Nice because it's going to speed up
> > things.
> > 
> > I think we should start doing ruleset generation tracking, IIRC this
> > is missing in the existing codebase. Such approach requires to keep
> > track of the ruleset generation number and invalidate caches.
> 
> Do you think parallel ruleset updates are a problem? None of the xtables
> tools are meant to stay running for long, so I didn't expect problems
> there until now. Sure, loading 10k rules with iptables-nft-restore takes
> a long time, but if kernel's ruleset changes in between, I don't see how
> we could recover from that (apart from restarting the whole restore).

Yes, restarting is the only way.

I was not thinking on the "load big ruleset" via -restore problem
(it's flushing the ruleset unless noflush option is used), instead
incremental updates could be? eg.  robot placing rules to be
added/deleted/replaced in a batch file that is passed to
iptables-restore to update an existing ruleset.

But in such case, the rules in a given chain are still consistent,
since we check for interference there already via EINTR in netlink
dumps. We just may end up caching chains with rules in different
generations with this new approach. Probably this may end up to
misleading error reporting in a heavy concurrent rules update
environment. We could still easily detect this by enforcing checks for
consistent generation idea (otherwise restart) and use
NFNL_BATCH_GENID attribute for "batch is based on ruleset generation
X semantics" (not used by userspace code yet). That could be done in a
follow up patchset if needed. We can probably get rid of the global
userspace lock for iptables at some point.

On Mon, Dec 17, 2018 at 08:25:24PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Dec 17, 2018 at 07:24:24PM +0100, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Mon, Dec 17, 2018 at 06:42:49PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Dec 13, 2018 at 12:15:56PM +0100, Phil Sutter wrote:
> > > > Use recently introduced support for rules inside chains in libnftnl to
> > > > introduce a rule cache per chain instead of a global one.
> > > > 
> > > > A tricky bit is to decide if cache should be updated or not. Previously,
> > > > the global rule cache was populated just once and then reused unless
> > > > being flushed completely (via call to flush_rule_cache() with
> > > > NULL-pointer table argument). Resemble this behaviour by introducing a
> > > > boolean indicating cache status and fetch rules for all chains when
> > > > updating the chain cache in nft_chain_list_get().
> > > 
> > > This is a rather large rewrite: Nice because it's going to speed up
> > > things.
> > > 
> > > I think we should start doing ruleset generation tracking, IIRC this
> > > is missing in the existing codebase. Such approach requires to keep
> > > track of the ruleset generation number and invalidate caches.
> > 
> > Do you think parallel ruleset updates are a problem? None of the xtables
> > tools are meant to stay running for long, so I didn't expect problems
> > there until now. Sure, loading 10k rules with iptables-nft-restore takes
> > a long time, but if kernel's ruleset changes in between, I don't see how
> > we could recover from that (apart from restarting the whole restore).
> 
> Yes, restarting is the only way.
> 
> I was not thinking on the "load big ruleset" via -restore problem
> (it's flushing the ruleset unless noflush option is used), instead
> incremental updates could be? eg.  robot placing rules to be
> added/deleted/replaced in a batch file that is passed to
> iptables-restore to update an existing ruleset.

Ah, I see. Do you think it's feasible to support that in iptables-nft
given that new users are probably better off if they use nftables
directly?

> But in such case, the rules in a given chain are still consistent,
> since we check for interference there already via EINTR in netlink
> dumps. We just may end up caching chains with rules in different
> generations with this new approach. Probably this may end up to
> misleading error reporting in a heavy concurrent rules update
> environment. We could still easily detect this by enforcing checks for
> consistent generation idea (otherwise restart) and use
> NFNL_BATCH_GENID attribute for "batch is based on ruleset generation
> X semantics" (not used by userspace code yet). That could be done in a
> follow up patchset if needed. We can probably get rid of the global
> userspace lock for iptables at some point.

Yes, indeed. My changes allow for fetching rules of a later generation
than the chains. At this point though, we either have a command which
doesn't need the rule cache or chain and rule fetching happens shortly
after each other. And once these have been fetched, no further cache
updates happen. We also have to be careful here, because at least my
patch fixing for wrong rule position when using '-I <NUM>' with
iptables-nft-restore heavily depends upon a chain which has been
removed/flushed earlier is not fetched again. Hence why I introduced
those booleans, basically avoiding cache updates despite a possible
asymmetry.

That global userspace lock you're talking about is within kernel space?
I'm asking because the locking in userspace is not required with
iptables-nft (as opposed to legacy).

Cheers, Phil

On Mon, Dec 17, 2018 at 10:11:30PM +0100, Phil Sutter wrote:
> On Mon, Dec 17, 2018 at 08:25:24PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Dec 17, 2018 at 07:24:24PM +0100, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Mon, Dec 17, 2018 at 06:42:49PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Dec 13, 2018 at 12:15:56PM +0100, Phil Sutter wrote:
> > > > > Use recently introduced support for rules inside chains in libnftnl to
> > > > > introduce a rule cache per chain instead of a global one.
> > > > > 
> > > > > A tricky bit is to decide if cache should be updated or not. Previously,
> > > > > the global rule cache was populated just once and then reused unless
> > > > > being flushed completely (via call to flush_rule_cache() with
> > > > > NULL-pointer table argument). Resemble this behaviour by introducing a
> > > > > boolean indicating cache status and fetch rules for all chains when
> > > > > updating the chain cache in nft_chain_list_get().
> > > > 
> > > > This is a rather large rewrite: Nice because it's going to speed up
> > > > things.
> > > > 
> > > > I think we should start doing ruleset generation tracking, IIRC this
> > > > is missing in the existing codebase. Such approach requires to keep
> > > > track of the ruleset generation number and invalidate caches.
> > > 
> > > Do you think parallel ruleset updates are a problem? None of the xtables
> > > tools are meant to stay running for long, so I didn't expect problems
> > > there until now. Sure, loading 10k rules with iptables-nft-restore takes
> > > a long time, but if kernel's ruleset changes in between, I don't see how
> > > we could recover from that (apart from restarting the whole restore).
> > 
> > Yes, restarting is the only way.
> > 
> > I was not thinking on the "load big ruleset" via -restore problem
> > (it's flushing the ruleset unless noflush option is used), instead
> > incremental updates could be? eg.  robot placing rules to be
> > added/deleted/replaced in a batch file that is passed to
> > iptables-restore to update an existing ruleset.
> 
> Ah, I see. Do you think it's feasible to support that in iptables-nft
> given that new users are probably better off if they use nftables
> directly?

Right indeed, nftables is better there, but you can also use
iptables-restore to do incremental ruleset updates with noflush option
in iptables.

> > But in such case, the rules in a given chain are still consistent,
> > since we check for interference there already via EINTR in netlink
> > dumps. We just may end up caching chains with rules in different
> > generations with this new approach. Probably this may end up to
> > misleading error reporting in a heavy concurrent rules update
> > environment. We could still easily detect this by enforcing checks for
> > consistent generation idea (otherwise restart) and use
> > NFNL_BATCH_GENID attribute for "batch is based on ruleset generation
> > X semantics" (not used by userspace code yet). That could be done in a
> > follow up patchset if needed. We can probably get rid of the global
> > userspace lock for iptables at some point.
> 
> Yes, indeed. My changes allow for fetching rules of a later generation
> than the chains. At this point though, we either have a command which
> doesn't need the rule cache or chain and rule fetching happens shortly
> after each other. And once these have been fetched, no further cache
> updates happen. We also have to be careful here, because at least my
> patch fixing for wrong rule position when using '-I <NUM>' with
> iptables-nft-restore heavily depends upon a chain which has been
> removed/flushed earlier is not fetched again. Hence why I introduced
> those booleans, basically avoiding cache updates despite a possible
> asymmetry.

Sounds fine.

> That global userspace lock you're talking about is within kernel space?
> I'm asking because the locking in userspace is not required with
> iptables-nft (as opposed to legacy).

Exactly. The userspace lock is backward compatibility stuff for scripts.

OK, please revamp and send v3, thanks for addressing my feedback.