[prev in list] [next in list] [prev in thread] [next in thread] 

List:       netfilter-devel
Subject:    Re: [iptables PATCH v3 11/21] xtables: Implement per chain rule cache
From:       Phil Sutter <phil () nwl ! cc>
Date:       2018-12-30 11:52:50
Message-ID: 20181230115250.GK17224 () orbyte ! nwl ! cc
[Download RAW message or body]

Hi Pablo,

On Thu, Dec 27, 2018 at 08:41:34PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 20, 2018 at 04:09:12PM +0100, Phil Sutter wrote:
> > @@ -1195,12 +1182,14 @@ err:
> >  	return NULL;
> >  }
> >  
> > -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h);
> > +static struct nftnl_chain *
> > +nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
> >  
> >  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;
> >  
> > @@ -1228,10 +1217,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);
> > +	if (c)
> > +		nftnl_chain_rule_add_tail(r, c);
> 
> I think we always have a chain here.

Not at this stage of the series: When only appending a rule in a single
arptables-nft command, the cache has not been populated. This is not a
problem per se, so we must allow for the above to fail. The old common
rule cache didn't require the actual chain to be in cache, that's why
this check is new.

> >  	return 1;
> 
> Because we return success in any case.
> 
> [...]
> > @@ -2024,16 +2028,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);
> > +	if (!c)
> 
> errno = ENOENT?
> 
> Or chain is assumed to be found always?

The check command is supported by ip(6)tables-nft only, do_parse() makes
sure the chain exists in final sanity checks. But yes, setting ENOENT
here would be more consistent so I'll send a follow-up which adds it.

[...]
> >  		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, r);
> >  		if (ret < 0)
> > @@ -2136,9 +2144,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);
> 
> I wonder why we need nftnl_chain_rule_insert_at() if there is no chain
> parameter, see below.

Yes, it is just to help readability - and of course because we need at
least *some* form of wrapper around list_add() due to the opaque struct
nftnl_rule.

[...]
> > @@ -2174,16 +2184,18 @@ 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;
> >  
> >  	nft_fn = nft_rule_replace;
> >  
> > -	list = nft_rule_list_get(h);
> > -	if (list == NULL)
> > +	c = nft_chain_find(h, table, chain);
> > +	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)
> 
> Here in nft_rule_replace() I can see we still use
> nftnl_rule_list_del() to remove an entry from the cache.
> 
> No problem, since internally nftnl_rule_list_del() does the same as
> nftnl_chain_rule_del().
> 
> We can probably deprecate nftnl_rule_list at some point, and rely
> entirely on nftnl_chain_rule_add() and so on.

Hmm. Yes, this is indeed a shortcoming of my patch to libnftnl: I did
not introduce a function to remove a rule from the chain's rule list.
I'll send a follow-up to fix this.

> [...]
> > @@ -3143,19 +3106,8 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
> >  	return -1;
> >  }
> >  
> > -struct nft_is_rule_compatible_data {
> > -	const char *tablename;
> > -};
> > -
> >  static int nft_is_rule_compatible(struct nftnl_rule *rule, void *data)
> >  {
> > -	const char *table = nftnl_rule_get_str(rule, NFTNL_RULE_TABLE);
> > -	struct nft_is_rule_compatible_data *d = data;
> > -
> > -	/* ignore rules belonging to a different table */
> > -	if (strcmp(table, d->tablename))
> > -		return 0;
> > -
> >  	return nftnl_expr_foreach(rule, nft_is_expr_compatible, NULL);
> >  }
> 
> This change I think it doesn't belong here, anyway, codebase looks
> better now, but for 'git annotate' purpose, better if we can avoid
> this in the future.

I disagree: With the introduction of per chain rule lists, rules are
implicitly assigned to their chain which in turn is implicitly assigned
to its table (due to the per table chain lists). Prior to that,
nft_is_rule_compatible() was called for all rules, also those belonging
to tables not part of the compat ruleset.

Well, I could have moved that change into a later patch since keeping
the check doesn't break things but I would have had to prepare a struct
nft_is_rule_compatible_data parameter in nft_is_chain_compatible() for
passing on to nftnl_rule_foreach() just to keep things working.

Cheers, Phil
[prev in list] [next in list] [prev in thread] [next in thread]