Re: [PATCH v5 1/2] netfilter: nf_tables: Implement jump limit for nft_table_validate

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

On Mon, May 19, 2025 at 11:08:41PM -0400, Shaun Brady wrote:
[...]
> @@ -4039,14 +4049,62 @@ int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
>  }
>  EXPORT_SYMBOL_GPL(nft_chain_validate);
>  
> -static int nft_table_validate(struct net *net, const struct nft_table *table)
> +/** nft_families_inc_jump - Determine if tables should add to the total jump
> + * count for a netns.
> + *
> + * @table: table of interest
> + * @sibling_table: a 'sibling' table to compare against
> + *
> + * Compare attributes of the tables to determine if the sibling tables
> + * total_jump_count should be added to the working context (done by caller).
> + * Mostly concerned with family compatibilities, but also identifying equality
> + * (a tables own addition will be recalculated soon).
> + *
> + * Ex: v4 tables do not apply to v6 packets
> + */
> +static bool nft_families_inc_jump(struct nft_table *table, struct nft_table *sibling_table)
> +{

I think we mentioned about NFPROTO_BRIDGE here too.

> +	/* Invert parameters to on require one test for two cases (the reverse) */
> +	if (sibling_table->family > table->family) /* include/uapi/linux/netfilter.h */
> +		return nft_families_inc_jump(sibling_table, table);
> +
> +	/* We found ourselves; don't add current jump count (will be counted dynamically) */
> +	if (sibling_table == table)
> +		return false;
> +
> +	switch (table->family) {
> +	case NFPROTO_IPV4:
> +		if (sibling_table->family == NFPROTO_IPV6)
> +			return false;
> +		break;
> +	}
> +
> +	return true;
> +}
> +
> +static int nft_table_validate(struct net *net, struct nft_table *table)

This function is called from abort path too. I suspect total_jump_count
for this table will not be OK in such case. And this selftest does cover
many cases.

>  {
>  	struct nft_chain *chain;
> +	struct nftables_pernet *nft_net;
>  	struct nft_ctx ctx = {
>  		.net	= net,
>  		.family	= table->family,
> +		.total_jump_count = 0,
>  	};
>  	int err;
> +	u32 total_jumps_before_validate;
> +	struct nft_table *sibling_table;
> +
> +	nft_net = nft_pernet(net);
> +
> +	if (!net_eq(net, &init_net)) {
> +		list_for_each_entry(sibling_table, &nft_net->tables, list) {
> +			if(nft_families_inc_jump(table, sibling_table))
                          ^
                  coding style

> +				ctx.total_jump_count += sibling_table->total_jump_count;
> +		}
> +	}
> +
> +	total_jumps_before_validate = ctx.total_jump_count;
>  
>  	list_for_each_entry(chain, &table->chains, list) {
>  		if (!nft_is_base_chain(chain))
> @@ -4060,6 +4118,8 @@ static int nft_table_validate(struct net *net, const struct nft_table *table)
>  		cond_resched();
>  	}
>  
> +	table->total_jump_count = ctx.total_jump_count - total_jumps_before_validate;
> +
>  	return 0;
>  }
>  
> @@ -4084,6 +4144,7 @@ int nft_setelem_validate(const struct nft_ctx *ctx, struct nft_set *set,
>  	case NFT_JUMP:
>  	case NFT_GOTO:
>  		pctx->level++;
> +		pctx->total_jump_count++;
>  		err = nft_chain_validate(ctx, data->verdict.chain);
>  		if (err < 0)
>  			return err;
> @@ -11916,6 +11977,59 @@ static struct notifier_block nft_nl_notifier = {
>  	.notifier_call  = nft_rcv_nl_event,
>  };
>  
> +#ifdef CONFIG_SYSCTL
> +static struct ctl_table nf_limit_control_sysctl_table[] = {
> +	{
> +		.procname	= "nf_max_table_jumps_netns",
> +		.data		= &init_net.nf.nf_max_table_jumps_netns,
> +		.maxlen		= sizeof(init_net.nf.nf_max_table_jumps_netns),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec,
> +	},
> +};
> +
> +static int netfilter_limit_control_sysctl_init(struct net *net)
> +{
> +	struct ctl_table *tbl;
> +
> +	net->nf.nf_max_table_jumps_netns = NFT_DEFAULT_MAX_TABLE_JUMPS;
> +	tbl = nf_limit_control_sysctl_table;
> +	if (!net_eq(net, &init_net)) {
> +		tbl = kmemdup(tbl, sizeof(nf_limit_control_sysctl_table), GFP_KERNEL);

Not checking error:

                if (!tbl)
                        ...

> +		tbl->data = &net->nf.nf_max_table_jumps_netns;
> +		if (net->user_ns != &init_user_ns)
> +			tbl->mode &= ~0222;
> +	}
> +
> +	net->nf.nf_limit_control_dir_header = register_net_sysctl_sz(
> +		net, "net/netfilter", tbl, ARRAY_SIZE(nf_limit_control_sysctl_table));
> +
> +	if (!net->nf.nf_limit_control_dir_header)
> +		goto err_alloc;
> +
> +	return 0;
> +
> +err_alloc:
> +	if (tbl != nf_limit_control_sysctl_table)
> +		kfree(tbl);
> +	return -ENOMEM;
> +}