Re: [PATCH net 07/10] netfilter: nf_tables: skip double clone set expressions on element insert

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

On Sat, Aug 22, 2026 at 12:52:28PM -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 double clone set expressions on element insert
> 
> This adds an override_exprs flag so that the dynset and newsetelem paths
> borrow the set's expressions instead of cloning them when userspace does
> not supply an override, and releases the expression array only when it
> really is an override.  The same flag is now used to decide whether the
> dynset expression override is dumped back to userspace.
> 
> > diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
> > index ee9d3e7b1ecf1..fa4da694e92d8 100644
> > --- a/net/netfilter/nft_dynset.c
> > +++ b/net/netfilter/nft_dynset.c
> > @@ -19,7 +19,8 @@ struct nft_dynset {
> >  	u8				sreg_key;
> >  	u8				sreg_data;
> >  	bool				invert;
> > -	bool				expr;
> > +	bool				expr:1,
> > +					override_exprs:1;
> >  	u8				num_exprs;
> >  	u64				timeout;
> >  	struct nft_expr			*expr_array[NFT_SET_EXPR_MAX];
> 
> [ ... ]
> 
> > @@ -393,7 +398,7 @@ static int nft_dynset_dump(struct sk_buff *skb,
> >  			 nf_jiffies64_to_msecs(priv->timeout),
> >  			 NFTA_DYNSET_PAD))
> >  		goto nla_put_failure;
> > -	if (priv->set->num_exprs == 0) {
> > +	if (priv->set->num_exprs == 0 || priv->override_exprs) {
> >  		if (priv->num_exprs == 1) {
> >  			if (nft_expr_dump(skb, NFTA_DYNSET_EXPR,
> >  					  priv->expr_array[0], reset))
> 
> [Severity: Medium]
> Can the message emitted by nft_dynset_dump() after this change be rejected
> by nft_dynset_init() when it is replayed?
> 
> With the widened condition, a dynset attached to a set that owns
> expressions (set->num_exprs > 0) now dumps its override.  For
> priv->num_exprs > 1 that override is emitted as an NFTA_DYNSET_EXPRESSIONS
> nest:
> 
> net/netfilter/nft_dynset.c:nft_dynset_dump() {
> 	...
> 	u32 flags = priv->invert ? NFT_DYNSET_F_INV : 0;
> 	...
> 	} else if (priv->num_exprs > 1) {
> 		struct nlattr *nest;
> 
> 		nest = nla_nest_start_noflag(skb, NFTA_DYNSET_EXPRESSIONS);
> 	...
> 	if (nla_put_be32(skb, NFTA_DYNSET_FLAGS, htonl(flags)))
> 	...
> }
> 
> The dumped NFTA_DYNSET_FLAGS is computed only from priv->invert, so
> NFT_DYNSET_F_EXPR is never set, even though priv->expr records that
> userspace passed it at creation time.  On the parse side the
> NFTA_DYNSET_EXPRESSIONS branch requires that flag:
> 
> net/netfilter/nft_dynset.c:nft_dynset_init() {
> 	...
> 	} else if (tb[NFTA_DYNSET_EXPRESSIONS]) {
> 		...
> 		if (!priv->expr)
> 			return -EINVAL;
> 	...
> }
> 
> So a rule with two override expressions on an NFT_SET_EVAL set that also
> has two set expressions (for example counter plus quota) appears to be
> dumped in a form that the kernel's own parser refuses with -EINVAL when the
> netlink message is sent back (raw netlink echo, or libnftnl parse-then-build
> restore paths).  Before this patch that configuration emitted no expression
> attributes at all, so the replay was accepted and silently fell back to the
> set's expressions.
> 
> The missing NFT_DYNSET_F_EXPR in the dump path predates this patch (it is
> already reachable for priv->set->num_exprs == 0 with more than one
> expression), but the new condition extends the asymmetry to
> override-on-eval-set rules that used to round-trip.  Should
> nft_dynset_dump() also report NFT_DYNSET_F_EXPR when priv->expr is set?

This sounds like a theotical issue:

1) Example with one single expression.

table ip x {
        set y {
                typeof ip saddr
                flags dynamic
                ct count over 2
        }
 
        chain z {
                update @y { ip saddr ct count over 3 }
        }
}

2) Example with two expressions

table ip x {
        set y {
                typeof ip saddr
                flags dynamic
                counter ct count over 2
        }
 
        chain z {
                update @y { ip saddr counter ct count over 3 }
        }
}

It works fine with userspace 'nft'.

It might be a correctness API issue to guarantee this symmetry, but we
can follow up.