Re: [PATCH nf-next,v1 6/6] netfilter: nf_tables: add support for validating incremental ruleset updates

[Phil Sutter <phil@nwl.cc>]

On Wed, May 14, 2025 at 11:42:16PM +0200, Pablo Neira Ayuso wrote:
> Use the new binding graph to validate incremental ruleset updates.
> 
> Perform full validation if the table is new, which is the case of the
> initial ruleset reload. Subsequent incremental ruleset updates use the
> validation list which contains chains with new rules or chains that can
> be now reached via jump/goto from another rule/set element.
> 
> When validating a chain from commit/abort phase, backtrack to collect
> the basechains that can reach this chain, then perform the validation of
> rules in this chain. If no basechains can be reached, then skip
> validation for this chain. However, if basechains are off the jump stack
> limit, then resort to full ruleset validation. This is to prevent
> inconsistent validation between the preparation and commit/abort phase
> validations.
> 
> As for loop checking, stick to the existing approach which uses the jump
> stack limit to detect cycles.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_tables_api.c | 188 +++++++++++++++++++++++++++++++++-
>  1 file changed, 183 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index e92cccc834d9..0f183abbc94f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -10397,6 +10397,160 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
>  	},
>  };
>  
> +struct basechain_array {
> +	const struct nft_chain	**basechain;
> +	uint32_t		num_basechains;
> +	uint32_t		max_basechains;
> +	int			max_level;
> +};
> +
> +static int basechain_array_add(struct basechain_array *array,
> +			       const struct nft_chain *chain, int level)
> +{
> +	const struct nft_chain **new_basechain;
> +	uint32_t new_max_basechains;
> +
> +	if (array->num_basechains == array->max_basechains) {
> +		new_max_basechains = array->max_basechains + 16;
> +		new_basechain = krealloc_array(array->basechain, new_max_basechains, sizeof(struct nft_chain *), GFP_KERNEL);
> +		if (!new_basechain)
> +			return -ENOMEM;
> +
> +		array->basechain = new_basechain;
> +		array->max_basechains = new_max_basechains;
> +	}
> +	array->basechain[array->num_basechains++] = chain;
> +
> +	if (level > array->max_level)
> +		array->max_level = level;
> +
> +	return 0;
> +}
> +
> +static int nft_chain_validate_backtrack(struct basechain_array *array,
> +					const struct list_head *backbinding_list,
> +					int *level)
> +{
> +	struct nft_binding *binding;
> +	int err;
> +
> +	/* Basechain is unreachable, fall back to slow path validation. */
> +	if (*level >= NFT_JUMP_STACK_SIZE)
> +		return -ENOENT;
> +
> +	list_for_each_entry(binding, backbinding_list, backlist) {
> +		if (binding->from.type == NFT_BIND_CHAIN &&
> +		    binding->from.chain->flags & NFT_CHAIN_BASE &&
> +		    binding->use > 0) {
> +			if (basechain_array_add(array, binding->from.chain, *level) < 0)
> +				return -ENOMEM;
> +
> +			continue;
> +		}
> +
> +		switch (binding->from.type) {
> +		case NFT_BIND_CHAIN:
> +			if (binding->use == 0)
> +				break;
> +
> +			(*level)++;
> +			err = nft_chain_validate_backtrack(array,
> +							   &binding->from.chain->backbinding_list,
> +							   level);
> +			if (err < 0)
> +				return err;
> +
> +			(*level)--;

It looks like you're trying to record the max level value for error
path, but I don't see it used in callers. Is this a leftover from before
introduction of basechain_array::max_level? If so, one may just pass
'level + 1' by value to the recursive call, right?

> +			break;
> +		case NFT_BIND_SET:
> +			if (binding->use == 0)
> +				break;
> +
> +			/* no level update for sets. */
> +			err = nft_chain_validate_backtrack(array,
> +							   &binding->from.set->backbinding_list,
> +							   level);
> +			if (err < 0)
> +				return err;
> +
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int nft_chain_validate_incremental(struct net *net,
> +					  const struct nft_chain *chain)
> +{
> +	struct basechain_array array = {};
> +	uint32_t i, level = 1;
> +	int err;
> +
> +	array.max_basechains = 16;
> +	array.basechain = kcalloc(16, sizeof(struct nft_chain *), GFP_KERNEL);
> +	if (!array.basechain)
> +		return -ENOMEM;
> +
> +	if (nft_is_base_chain(chain)) {
> +		err = basechain_array_add(&array, chain, 0);
> +		if (err < 0) {
> +			kfree(array.basechain);
> +			return -ENOMEM;
> +		}
> +	} else {
> +		err = nft_chain_validate_backtrack(&array,
> +						   &chain->backbinding_list,
> +						   &level);
> +		if (err < 0) {
> +			kfree(array.basechain);
> +			return err;
> +		}
> +	}
> +
> +	for (i = 0; i < array.num_basechains; i++) {
> +		struct nft_ctx ctx = {
> +			.net	= net,
> +			.family	= chain->table->family,
> +			.table	= chain->table,
> +			.chain	= (struct nft_chain *)array.basechain[i],
> +			.level	= array.max_level,
> +		};
> +
> +		if (WARN_ON_ONCE(!nft_is_base_chain(array.basechain[i])))
> +			continue;
> +
> +		err = nft_chain_validate(&ctx, chain);
> +		if (err < 0)
> +			break;
> +	}
> +
> +	kfree(array.basechain);
> +
> +	return err;
> +}
> +
> +static int nft_validate_incremental(struct net *net, struct nft_table *table)
> +{
> +	struct nftables_pernet *nft_net = nft_pernet(net);
> +	struct nft_chain *chain, *next;
> +	int err;
> +
> +	err = 0;
> +	list_for_each_entry_safe(chain, next, &nft_net->validate_list, validate_list) {
> +		if (chain->table != table)
> +			continue;
> +
> +		if (err >= 0)
> +			err = nft_chain_validate_incremental(net, chain);

Is there a future use-case for err > 0? I don't see where that function
would return a positive value. If this is not to change, I guess just
checking err || !err would simplify things a bit and also avoid the
EINTR != !EINTR pitfall (see below).

> +
> +		list_del(&chain->validate_list);
> +		chain->validate = 0;
> +	}
> +
> +	return err;
> +}
> +
>  static void nft_validate_chain_release(struct net *net)
>  {
>  	struct nftables_pernet *nft_net = nft_pernet(net);
> @@ -10422,12 +10576,36 @@ static int nf_tables_validate(struct net *net)
>  			nft_validate_state_update(table, NFT_VALIDATE_DO);
>  			fallthrough;
>  		case NFT_VALIDATE_DO:
> -			err = nft_table_validate(net, table);
> -			if (err < 0) {
> -				if (err == EINTR)

This should be -EINTR, right?

> -					goto err_eintr;
> +			/* If this table is new, then this is the initial
> +			 * ruleset restore, perform full table validation,
> +			 * otherwise, perform incremental validation.
> +			 */
> +			if (!nft_is_active(net, table)) {
> +				err = nft_table_validate(net, table);
> +				if (err < 0) {
> +					if (err == EINTR)

Same here?

> +						goto err_eintr;
>  
> -				return -EAGAIN;
> +					return -EAGAIN;
> +				}
> +			} else {
> +				err = nft_validate_incremental(net, table);
> +				if (err < 0) {
> +					if (err != -ENOMEM && err != -ENOENT)
> +						return -EAGAIN;
> +
> +					/* Either no memory or it cannot reach
> +					 * basechain, then fallback to full
> +					 * validation.
> +					 */
> +					err = nft_table_validate(net, table);
> +					if (err < 0) {
> +						if (err == EINTR)

And here as well?

Cheers, Phil

> +							goto err_eintr;
> +
> +						return -EAGAIN;
> +					}
> +				}
>  			}
>  			nft_validate_state_update(table, NFT_VALIDATE_SKIP);
>  			break;
> -- 
> 2.30.2
> 
> 
>