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

[Shaun Brady <brady.1345@gmail.com>]

On Tue, Jul 22, 2025 at 04:11:36AM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 19, 2025 at 11:08:41PM -0400, Shaun Brady wrote:
> > +static bool nft_families_inc_jump(struct nft_table *table, struct nft_table *sibling_table)
> > +{
> 
> I think we mentioned about NFPROTO_BRIDGE here too.
> 

NFPROTO_BRIDGE was indeed brought up, which brought us to the current
implementation of nft_families_inc_jump.


---- excerpt ----
> Maybe it is better to have a global limit for all tables, regardless
> the family, in a non-init-netns?

Looks like it would be simpler.

The only cases where processing is disjunct is ipv4 vs. ipv6.

---- /excerpt ----

The current implementation includes all tables which are not:
1) the same table we're working with (tbl == other_tbl) (self)
2) tables where the protocol is tbl.family = v4, other_tbl.family = v6
(and vice versa)

I believe from this jumps from NFPROTO_BRIDGE to high protocols will be
counted.

> > +
> > +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.

The abort case is something I did not consider.  Would
nft_table_validate be called after a transaction rollback, and thusly a
likely set of tables that previously validated successfully?

Another potential save is that I don't update nft_table.total_jump_count
until after a successful validation of the table.

I might ask, how is the abort state different for validation, or is the
abort state when someone literally kills an update (and thus would be a
rollback?)?

Would a test case that attempts to demonstrate safety of the value
suffice?

> 
> > +			if(nft_families_inc_jump(table, sibling_table))
>                           ^
>                   coding style

I see it (or really, don't).  Will fix in v6.

> 
> > +	if (!net_eq(net, &init_net)) {
> > +		tbl = kmemdup(tbl, sizeof(nf_limit_control_sysctl_table), GFP_KERNEL);
> 
> Not checking error:
> 
>                 if (!tbl)
>                         ...

Good catch.  I think I can jump down to the same err_alloc label below.


Thanks for the feedback!


SB