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
