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

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