Re: [PATCH net 10/10] netfilter: nf_tables: skip expired catchall elements on insert and delete

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

On Thu, Sep 17, 2026 at 07:04:12PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: nf_tables: skip expired catchall elements on insert and delete
> 
> This adds an expiry test to the catchall duplicate walk in
> nft_setelem_catchall_insert() and to the delete-candidate walk in
> nft_setelem_catchall_deactivate(), using the per-netns transaction
> timestamp.  The stated goal is to make both control-plane walks agree with
> the lookup and dump predicates.
> 
> A couple of questions on the new predicates and on the commit message.
> 
> > The delete path shows the same drift: nft_setelem_catchall_deactivate()
> > picks the first active-next entry in the catchall list, so with an
> > expired entry still pending GC it retires the stale entry instead of
> > the fresh one, and it deactivates an element that userspace no longer
> > sees instead of failing with -ENOENT.
> 
> [Severity: Low]
> Is the first half of this paragraph reachable before this patch?  The
> "expired entry still pending GC ... instead of the fresh one" wording
> implies a stale active-next entry and a fresh active-next entry can coexist
> in catchall_list.
> 
> Pre-patch nft_setelem_catchall_insert() rejected every entry that was
> active in the next generation, expired or not:
> 
> 	list_for_each_entry(catchall, &set->catchall_list, list) {
> 		ext = nft_set_elem_ext(set, catchall->elem);
> 		if (nft_set_elem_active(ext, genmask)) {
> 			*priv = catchall->elem;
> 			return -EEXIST;
> 		}
> 	}
> 
> The other ways an entry leaves active-next all unlink it from
> catchall_list (nft_trans_gc_catchall_sync(), the async GC completion) or
> happen on an explicit delete, and the non-EXCL add refreshed the existing
> element through nft_trans_elem_update() rather than linking a second one.
> So the "wrong entry retired" state seems to first become reachable because
> of the insert hunk in this same patch.
> 
> The second half (deactivating an element userspace no longer sees instead of
> returning -ENOENT) does look pre-existing.
> 
> Since the two hunks are interdependent, could the message say so explicitly
> so the stable backport is not split?

No need to split this stable backport...

> [Severity: High]
> Should this predicate also test nft_set_elem_is_dead()?

Yes, originally this patch checked for the dead and it is indeed need.