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
