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

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.

quoted

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.