Re: [PATCH 2/9] netfilter/nf_tables: Add folio_scratchpad collector to struct nftables_pernet

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Asynchronous reset and synchronous abort paths cause severe use-after-free, memory corruption, and spinlock corruption due to sharing a single per-net scratchpad across overlapping batch lifecycles.
- [High] Transaction batches that consist exclusively of creation operations permanently leak the memory used for their `nft_trans` objects.
- [Medium] Re-initialization of the scratchpad during an abort drops the feature-toggle static key and hardcodes the allocation order, causing memory over-allocation on some architectures.
--

--- Patch [2]: [PATCH 2/9] netfilter/nf_tables: Add folio_scratchpad collector to struct nftables_pernet ---
commit 210be9d0ffcaa1541a9b10d23109de51629b6996
Author: Jim Cromie <[email protected]>

netfilter/nf_tables: Add folio_scratchpad collector to struct nftables_pernet

This commit introduces a folio_scratchpad to struct nftables_pernet to handle
variable-sized transaction object allocations. It aims to improve allocation
and teardown speed by replacing standard SLUB allocations with bulk teardown.

> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> [ ... ]
> @@ -221,7 +238,7 @@ static void nft_trans_list_del(struct nft_trans *trans)
>  static void nft_trans_destroy(struct nft_trans *trans)
>  {
>         nft_trans_list_del(trans);
> -       kfree(trans);
> +       nft_trans_free(trans);
>  }

[Severity: High]
During commit for creation operations, nft_trans_destroy() unlinks the
transaction and calls nft_trans_free(), which is a no-op for scratchpad
chunks.

By the time the code reaches nf_tables_commit_release(), the commit_list is
completely empty.

This causes nf_tables_commit_release() to return early and skip scheduling
the destroy worker. Since the asynchronous worker never runs,
folio_scratchpad_reset() is never called.

Can batches consisting only of creation operations permanently leak memory?

> [ ... ]
> @@ -10267,6 +10286,8 @@ static void nf_tables_trans_destroy_work(struct work_struct *w)
>                 nft_trans_list_del(trans);
>                 nft_commit_release(trans);
>         }
> +
> +       folio_scratchpad_reset(&nft_net->trans_scratchpad);
>  }

[Severity: Critical]
When nf_tables_commit_release() schedules nf_tables_trans_destroy_work()
and drops the commit_mutex, a new batch can immediately begin allocating
transactions from this exact same scratchpad.

Meanwhile, the asynchronous worker for the previous batch will unconditionally
call folio_scratchpad_reset() here.

This rewinds the allocator's free pointer and destroys the memory actively
being used by the new batch.

Does this shared per-net scratchpad suffer from a race condition between
concurrent batches?

> [ ... ]
> @@ -11491,6 +11514,9 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb,
>         else
>                 nf_tables_module_autoload_cleanup(net);
>  
> +       folio_scratchpad_free(&nft_net->trans_scratchpad);
> +       folio_scratchpad_init(&nft_net->trans_scratchpad, 4);
> +
>         mutex_unlock(&nft_net->commit_mutex);

[Severity: Critical]
If the first batch's async worker is currently accessing the folios or
waiting on the internal scratchpad spinlock, calling
folio_scratchpad_free() and folio_scratchpad_init() here will free the
underlying folios and re-initialize the spinlock.

Does this synchronous abort corrupt the allocator state if the previous
batch's asynchronous worker is still running?

[Severity: Medium]
In nf_tables_init_net(), the scratchpad is initialized properly using
folio_scratchpad_init_key() with get_order(SZ_64K) and the
nft_trans_scratchpad_key.

Passing the literal 4 here instead of get_order(SZ_64K) forces the
allocator to request 1MB pages on systems with 64KB pages.

Additionally, folio_scratchpad_init() sets sp->key = NULL, which
permanently strips the feature toggle validation from all subsequent
allocations in this network namespace.

Does this re-initialization hardcode the allocation order and drop the
feature toggle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2