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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The relaxed duplicate predicate in
  nft_setelem_catchall_insert() (nf_tables_api.c:7003-7016) removes the…

--

netfilter: nf_tables: skip expired catchall elements on insert and delete

This aligns nft_setelem_catchall_insert() and
nft_setelem_catchall_deactivate() with the lookup and dump predicates, so
an expired catchall element no longer counts as a duplicate and no longer
gets picked as the delete candidate. Both walks now also test
__nft_set_elem_expired() with the per-netns transaction timestamp and
nft_set_elem_is_dead().

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c0b754a2d45b..b59628e6240c 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6995,11 +6995,14 @@ 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) &&
> +		    !nft_set_elem_is_dead(ext)) {
>  			*priv = catchall->elem;
>  			return -EEXIST;
>  		}
>  	}

[Severity: Medium]
Can repeated expire-then-readd cycles grow set->catchall_list without
bound here?

With the relaxed predicate, the expired entry is skipped but stays linked
and genmask-active until GC runs, so the tail of
nft_setelem_catchall_insert() appends a second entry alongside it:

	catchall = kmalloc_obj(*catchall, GFP_KERNEL_ACCOUNT);
	if (!catchall)
		return -ENOMEM;

	catchall->elem = elem->priv;
	list_add_tail_rcu(&catchall->list, &set->catchall_list);

Unlike ordinary elements, catchall elements are excluded from the 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;
	}

so the "return set_full ? -ENFILE : 0;" limiter that caps accumulation of
ordinary elements does not apply to this path. Is there anything else that
bounds the number of stale catchall entries a set can hold?

Reclamation looks like it depends entirely on GC, and the interval is
user-tunable with no upper bound (NFTA_SET_GC_INTERVAL is accepted as a u32
in milliseconds and nft_set_gc_interval() just converts it):

include/net/netfilter/nf_tables.h:nft_set_gc_interval() {
	u32 gc_int = READ_ONCE(set->gc_int);

	return gc_int ? msecs_to_jiffies(gc_int) : HZ;
}

For nft_set_rhash and nft_set_rbtree the catchall reclaim runs only from
the delayed gc work requeued with that value, so with a large gc-interval
the window for accumulation is correspondingly large.

There is also a packet-path effect: nft_set_catchall_lookup() walks the
whole list, and new elements are appended at the tail, so every accumulated
stale entry is dereferenced before the live one is found:

	list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
		ext = nft_set_elem_ext(set, catchall->elem);
		if (nft_set_elem_active(ext, genmask) &&
		    !nft_set_elem_expired(ext) &&
		    !nft_set_elem_is_dead(ext))
			return ext;
	}

Would it be preferable to reuse or force-reclaim the stale catchall element
on insert, rather than only ignoring it?

> @@ -7092,11 +7095,14 @@ 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) ||
> +		    nft_set_elem_is_dead(ext))
>  			continue;
>  
>  		kfree(elem->priv);

[Severity: Medium]
Related to the above: with stale entries now skipped and -ENOENT returned,
per-element DELSETELEM can no longer retire the accumulated entries, so a
full set flush or set deletion appears to be the only way to drop them
before GC fires. Is that the intended behaviour?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918112844.194503-2-pablo%40netfilter.org