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

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

Hi Jakub,

This comments below look like brainstorming, nothing really that
should prevent this patch from being merged.

On Sat, Sep 19, 2026 at 01:34:51PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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():

Set size is specified by user... User can create a set without a set
size... This comment makes no sense.

> 	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?

We could sets a cap for maximum number of items in a transaction, but
noone requested this so far. There is also memcg which should provide
a cap.

> 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?

Maybe, but that is unrelated to this patch.

> > @@ -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