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

[Jakub Kicinski <kuba@kernel.org>]

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?

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c0b754a2d45b0..800efb4d91ebe 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6995,11 +6995,13 @@ static int nft_setelem_catchall_insert(const struct net *net,
>  {
>  	struct nft_set_elem_catchall *catchall;
>  	u8 genmask = nft_genmask_next(net);
> +	u64 tstamp = nft_net_tstamp(net);
>  	struct nft_set_ext *ext;
>  
>  	list_for_each_entry(catchall, &set->catchall_list, list) {
>  		ext = nft_set_elem_ext(set, catchall->elem);
> -		if (nft_set_elem_active(ext, genmask)) {
> +		if (nft_set_elem_active(ext, genmask) &&
> +		    !__nft_set_elem_expired(ext, tstamp)) {
>  			*priv = catchall->elem;
>  			return -EEXIST;
>  		}

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

The commit message says the walk is aligned "with the lookup and dump
predicates" and is "in line with the set backend .insert/.deactivate", but
both of those references carry a dead test that is missing here.
nft_set_catchall_lookup() in the same file:

	if (nft_set_elem_active(ext, genmask) &&
	    !nft_set_elem_expired(ext) &&
	    !nft_set_elem_is_dead(ext))
		return ext;

and nft_rhash_cmp() in nft_set_hash.c, which is what the rhash
.insert/.deactivate comparisons go through:

	if (nft_set_elem_is_dead(&he->ext))
		return 1;
	if (__nft_set_elem_expired(&he->ext, x->tstamp))
		return 1;

The two predicates can disagree because the DEAD bit is set from the async
GC worker against real jiffies, while the control plane compares against
nft_net->tstamp, latched once per netlink batch in nf_tables_valid_genid():

	nft_net->tstamp = get_jiffies_64();

An element created inside the current batch always has
expiration = get_jiffies_64() + timeout, i.e. later than tstamp.  If its
timeout (which may be as small as 1 ms) elapses while the same batch is
still being processed, nft_trans_gc_catchall_async() marks it DEAD, yet
__nft_set_elem_expired(ext, tstamp) is still false here.

Does that leave the element in a state where nft_set_catchall_lookup()
skips it for every packet, but this walk still returns -EEXIST?  With
NLM_F_EXCL the re-add then fails for an element that no longer matches
anything.

Without NLM_F_EXCL, the -EEXIST path in nft_add_set_elem() queues
NFT_TRANS_UPD_TIMEOUT/EXPIRATION and nft_trans_elem_update() rewrites the
expiration:

	if (update->flags & NFT_TRANS_UPD_EXPIRATION)
		WRITE_ONCE(nft_set_ext_timeout(ext)->expiration, get_jiffies_64() + update->expiration);

Nothing clears NFT_SET_ELEM_DEAD_BIT anywhere (the only sites are set_bit()
in nft_trans_gc_catchall_async() and in nft_set_hash.c), and
nft_trans_gc_catchall_async() starts with

		if (!nft_set_elem_expired(ext))
			continue;

so after the refresh the element is neither matched by the datapath nor
reclaimed by GC.  A userspace loop that keeps refreshing the timeout would
keep it in that state indefinitely.  Is that not the same "catchall rule may
silently stop matching for an arbitrarily long time" symptom the patch sets
out to remove?  nft_set_catchall_dump() and nft_setelem_catchall_get() also
lack the dead test, so the element is still listed to userspace while never
matching traffic.

>  	}
>  
>  	catchall = kmalloc_obj(*catchall, GFP_KERNEL_ACCOUNT);
>  	if (!catchall)
>  		return -ENOMEM;
>  
>  	catchall->elem = elem->priv;
>  	list_add_tail_rcu(&catchall->list, &set->catchall_list);

[Severity: High]
Can this grow set->catchall_list without bound?

The skipped stale entry is neither unlinked nor reused, so the function
falls through to kmalloc_obj() + list_add_tail_rcu() and appends a new
entry while the expired one stays linked.  Repeating "wait for the element
timeout to elapse, then NFT_MSG_NEWSETELEM with NFT_SET_ELEM_CATCHALL"
appends one entry per cycle.

Nothing appears to cap that, since catchall elements are excluded from the
set size accounting in nft_add_set_elem():

	if (!(flags & NFT_SET_ELEM_CATCHALL)) {
		unsigned int max = nft_set_maxsize(set), nelems;

		nelems = atomic_inc_return(&set->nelems);
		if (nelems > max)
			set_full = true;
	}

and the decrement side skips them too, in nft_trans_gc_trans_free():

		if (!nft_setelem_is_catchall(trans->set, elem_priv))
			atomic_dec(&trans->set->nelems);

so nft_set_maxsize() and -ENFILE never apply here.  For timeout-capable
hash sets the only reaper is the delayed nft_rhash_gc() work:

	queue_delayed_work(system_power_efficient_wq, &priv->gc_work,
			   nft_set_gc_interval(set));

and gc_int comes straight from NFTA_SET_GC_INTERVAL in nf_tables_newset()
with no upper bound, so the same requester controls both how fast entries
accumulate (1 ms element timeout) and how long they stay linked.  The GC
batch is additionally dropped when gc_seq moves, so a stream of small
batches can keep starving reclamation.

Besides the memory, does this not also make the data path scale with the
number of retained entries?  nft_set_catchall_lookup() walks the whole list
under rcu_read_lock() for every packet, and
nft_setelem_catchall_get()/nft_set_catchall_dump() walk it as well.

Would retiring or reusing the skipped element in this walk, rather than
just skipping it, avoid both problems?

>  
>  	return 0;
>  }

[ ... ]

> @@ -7092,11 +7094,13 @@ static int nft_setelem_catchall_deactivate(const struct net *net,
>  					   struct nft_set_elem *elem)
>  {
>  	struct nft_set_elem_catchall *catchall;
> +	u64 tstamp = nft_net_tstamp(net);
>  	struct nft_set_ext *ext;
>  
>  	list_for_each_entry(catchall, &set->catchall_list, list) {
>  		ext = nft_set_elem_ext(set, catchall->elem);
> -		if (!nft_is_active_next(net, ext))
> +		if (!nft_is_active_next(net, ext) ||
> +		    __nft_set_elem_expired(ext, tstamp))
>  			continue;

[Severity: High]
Same question for the delete side: should this skip condition also include
nft_set_elem_is_dead(ext)?  A DEAD element that the datapath already ignores
in nft_set_catchall_lookup() is still selected here and deactivated, where
-ENOENT looks like the expected result.

>  
>  		kfree(elem->priv);