Re: [PATCH net-next 09/12] netfilter: nft_ct: move custom expectation support to helper

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

On Tue, Aug 11, 2026 at 06:34:46PM -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: nft_ct: move custom expectation support to helper
> 
> This moves the nft ct expectation support from creating the expectation
> directly in the object eval path to allocating an internal (unregistered)
> conntrack helper that is attached to the conntrack, so the expectation is
> created from the helper callback at confirmation time. A new exported
> nf_conntrack_helper_release() is added to tear down that internal helper.
> 
> Link: https://patch.msgid.link/20260715144755.00ea7dfcd9f@proton.me
> 
> A few questions below.
> 
> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> > index 506c580347615..c30ae3f203bee 100644
> > --- a/net/netfilter/nf_conntrack_helper.c
> > +++ b/net/netfilter/nf_conntrack_helper.c
> > @@ -448,6 +448,15 @@ static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
> >  	return this == me;
> >  }
> >  
> > +void nf_conntrack_helper_release(struct nf_conntrack_helper *me)
> > +{
> > +	nf_ct_expect_iterate_destroy(expect_iter_me, me);
> > +
> > +	if (refcount_dec_and_test(&me->ct_refcnt))
> > +		kfree_rcu(me, rcu);
> > +}
> > +EXPORT_SYMBOL_GPL(nf_conntrack_helper_release);
> > +
> >  void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> >  {
> >  	mutex_lock(&nf_ct_helper_mutex);
> > @@ -463,10 +472,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> >  	 */
> >  	synchronize_rcu();
> >  
> > -	nf_ct_expect_iterate_destroy(expect_iter_me, me);
> > -
> > -	if (refcount_dec_and_test(&me->ct_refcnt))
> > -		kfree_rcu(me, rcu);
> > +	nf_conntrack_helper_release(me);
> >  }
> >  EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
> >  
> > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > index 358b9287e12e8..9dbf127df9c81 100644
> > --- a/net/netfilter/nft_ct.c
> > +++ b/net/netfilter/nft_ct.c
> 
> [ ... ]
> 
> > @@ -1226,6 +1228,93 @@ static int nft_ct_expect_timeout_get(const struct nlattr *attr, u32 *val)
> >  	return 0;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_NF_NAT)
> > +static void nft_ct_nat_follow_master(struct nf_conn *ct, struct nf_conntrack_expect *this)
> > +{
> > +	const struct nf_ct_helper_expectfn *expfn;
> > +
> > +	expfn = nf_ct_helper_expectfn_find_by_name("nat-follow-master");
> > +	if (expfn)
> > +		expfn->expectfn(ct, this);
> > +}
> > +#endif
> > +
> > +struct nft_ct_expect_data {
> > +	struct nft_ct_expect_obj	obj;
> > +	enum ip_conntrack_dir		dir;
> > +};
> > +
> > +static int ct_expect_help(struct sk_buff *skb, unsigned int protoff,
> > +			  struct nf_conn *ct, enum ip_conntrack_info ctinfo)
> > +{
> > +	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
> > +	struct nft_ct_expect_data *expect_data;
> > +	struct nf_conntrack_expect *exp;
> > +	int ret = NF_ACCEPT;
> > +	u16 l3num;
> > +
> > +	if (nf_ct_is_confirmed(ct))
> > +		return NF_ACCEPT;
> > +
> > +	expect_data = nfct_help_data(ct);
> > +	if (!expect_data)
> > +		return NF_ACCEPT;
> > +
> > +	if (expect_data->dir != dir)
> > +		return NF_ACCEPT;
> > +
> > +	exp = nf_ct_expect_alloc(ct);
> > +	if (!exp)
> > +		return NF_DROP;
> > +
> > +	if (expect_data->obj.l3num == NFPROTO_INET)
> > +		l3num = nf_ct_l3num(ct);
> > +	else
> > +		l3num = expect_data->obj.l3num;
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but can obj.l3num ever be
> NFPROTO_INET here?  nft_ct_expect_obj_init() rejects it:
> 
> 	case NFPROTO_INET: /* tuple.src.l3num supports NFPROTO_IPV4/6 only */
> 	default:
> 		return -EAFNOSUPPORT;
> 
> so priv->l3num, and therefore the copy in expect_data->obj.l3num, can never
> hold NFPROTO_INET and the nf_ct_l3num(ct) branch looks unreachable.  The same
> code existed in the old nft_ct_expect_obj_eval() and was moved verbatim, so
> this is only a cleanup note: either drop the branch or let init accept
> NFPROTO_INET and resolve it per packet.

Pre-existing issue. As the patch describes, this existing expectation
support usecase is limited to ip and ip6. Not sure I will follow up,
I would like to hear about possible usecases given this is very
limited due to expect_clash() returning EBUSY in case an expectation
already exists.

> > +
> > +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, l3num,
> > +			  &ct->tuplehash[!dir].tuple.src.u3,
> > +			  &ct->tuplehash[!dir].tuple.dst.u3,
> > +			  expect_data->obj.l4proto, NULL, &expect_data->obj.dport);
> > +	exp->timeout += expect_data->obj.timeout;
> > +
> > +#if IS_ENABLED(CONFIG_NF_NAT)
> > +	if (ct->status & IPS_NAT_MASK) {
> > +		exp->saved_proto.tcp.port = expect_data->obj.dport;
> > +		exp->dir = !dir;
> > +		exp->expectfn = nft_ct_nat_follow_master;
> > +	}
> > +#endif
> > +	if (nf_ct_expect_related(exp, 0) != 0)
> > +		ret = NF_ACCEPT;
> > +
> > +	nf_ct_expect_put(exp);
> > +
> > +	return ret;
> > +}
> > +
> > +static int nft_ct_expect_helper_alloc(struct nft_ct_expect_obj *priv)
> > +{
> > +	struct nf_conntrack_helper *ct_expect_helper;
> > +
> > +	ct_expect_helper = kzalloc_obj(struct nf_conntrack_helper,
> > +				       GFP_KERNEL_ACCOUNT);
> > +	if (!ct_expect_helper)
> > +		return -ENOMEM;
> > +
> > +	snprintf(ct_expect_helper->name, sizeof(ct_expect_helper->name), "%s",
> > +		 "nft_ct_expect");
> > +	ct_expect_helper->me = THIS_MODULE;
> > +	ct_expect_helper->expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected = priv->size;
> > +	rcu_assign_pointer(ct_expect_helper->help, ct_expect_help);
> > +	refcount_set(&ct_expect_helper->ct_refcnt, 1);
> > +
> > +	/* No need to register this helper, this is internal. */
> > +	priv->helper = ct_expect_helper;
> > +
> > +	return 0;
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -1272,13 +1363,31 @@ static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
> >  
> >  	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
> >  	priv->size = nla_get_u8(tb[NFTA_CT_EXPECT_SIZE]);
> > +	if (!priv->size)
> > +		priv->size = NF_CT_EXPECT_MAX_CNT;
> 
> [Severity: Medium]
> Does this break the round-trip of NFTA_CT_EXPECT_SIZE?  priv->size is what
> nft_ct_expect_obj_dump() echoes back:
> 
> 	    nla_put_u8(skb, NFTA_CT_EXPECT_SIZE, priv->size))
> 
> so an object created with size 0 is now listed as size 255.  The policy
> [NFTA_CT_EXPECT_SIZE] = { .type = NLA_U8 } accepts 0, so this is reachable
> from userspace.
> 
> The meaning of size 0 also flips: the pre-patch eval tested
> 
> 	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size)
> 
> which is always true for 0, i.e. size 0 meant "never create expectations",
> whereas it now means "allow the maximum".
> 
> Would it work to apply the substitution only where it is needed, i.e. to
> expect_policy[NF_CT_EXPECT_CLASS_DEFAULT].max_expected in
> nft_ct_expect_helper_alloc(), and keep the user-supplied value in
> priv->size?  That is what __nf_conntrack_helper_register() does, but there
> the clamped value lives in an internal policy struct that is never sent to
> userspace.
> 
> Related: with the new scheme ct_expect_help() only runs while the conntrack
> is unconfirmed and only for the single recorded direction, so at most one
> expectation per conntrack is created and the size attribute has no
> observable effect anymore.

This is explaining the existing limitations in this nft_ct expectation
support.

The main goal with this patch is to address a real issue in the
conntrack core as the patch describes, which is more important than
this feature.

> > +
> > +	err = nf_ct_netns_get(ctx->net, ctx->family);
> > +	if (err < 0)
> > +		return err;
> >  
> > -	return nf_ct_netns_get(ctx->net, ctx->family);
> > +	err = nft_ct_expect_helper_alloc(priv);
> > +	if (err < 0) {
> > +		nf_ct_netns_put(ctx->net, ctx->family);
> > +		return err;
> > +	}
> > +
> > +	return err;
> >  }
> >  
> >  static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
> > -				       struct nft_object *obj)
> > +				      struct nft_object *obj)
> >  {
> > +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> > +	struct nf_conntrack_helper *me = priv->helper;
> > +
> > +	/* This helper is going away, disable it. */
> > +	rcu_assign_pointer(me->help, NULL);
> > +	nf_conntrack_helper_release(me);
> >  	nf_ct_netns_put(ctx->net, ctx->family);
> >  }
> 
> [Severity: High]
> Is a grace period needed between clearing me->help and sweeping the
> expectation table here?  nf_conntrack_helper_unregister() does exactly these
> two steps with a synchronize_rcu() in between:
> 
> 	rcu_assign_pointer(me->help, NULL);
> 
> 	/* Make sure every nothing is still using the helper unless its a
> 	 * connection in the hash.
> 	 */
> 	synchronize_rcu();
> 
> 	nf_conntrack_helper_release(me);
> 
> The refactor left the grace period in the unregister wrapper, so the new
> external caller does not get it.
> 
> Can an in-flight ct_expect_help() then insert an expectation after the sweep
> has finished?
> 
> CPU A (softirq), nf_confirm():
> 	helper_cb = rcu_dereference(helper->help);	/* still ct_expect_help */
> 	...
> 	ret = helper_cb(skb, protoff, ct, ctinfo);
> 	  ct_expect_help()
> 	    nf_ct_expect_alloc() / nf_ct_expect_init()
> 
> CPU B (process context), object delete:
> 	nft_ct_expect_obj_destroy()
> 	  rcu_assign_pointer(me->help, NULL);
> 	  nf_conntrack_helper_release()
> 	    nf_ct_expect_iterate_destroy(expect_iter_me, me);  /* sweeps */
> 
> CPU A continues:
> 	    nf_ct_expect_related(exp, 0)	/* takes nf_conntrack_expect_lock
> 						   after the sweep, inserts */
> 
> rcu_assign_pointer() does not wait for a reader that already loaded the old
> pointer, and the callback is reached through the conntrack's own
> help->helper in nf_confirm() (and nf_ct_helper() in nf_conntrack_ovs.c), not
> through the nft object, so the nf_tables commit-time synchronize_rcu() that
> precedes ops->destroy() does not fence it.
> 
> Since this sweep is the only teardown for these expectations, would the
> escaped expectation (exp->helper == me, and exp->expectfn ==
> nft_ct_nat_follow_master when NAT is in use) then keep admitting a RELATED
> connection after the object/rule is gone, until its timeout expires or the
> master conntrack dies?
> 
> The commit message says:
> 
>     The nft_ct module removal always waits for rcu grace period, then the
>     NULL helper callback is observed after this.
> 
> but nft_ct_expect_obj_destroy() itself does not wait; object destruction is
> not module removal.  Would keeping the unregister ordering here (or folding
> the synchronize_rcu() into nf_conntrack_helper_release()) be preferable?

I can follow harden this, to kill existing expectations on module
removal for nft_ct and nfnetlink_cthelper, this race is unlikely to
happen but it is possible on module removal.

> > @@ -1297,27 +1406,14 @@ static int nft_ct_expect_obj_dump(struct sk_buff *skb,
> 
> [ ... ]
> 
> >  static void nft_ct_expect_obj_eval(struct nft_object *obj,
> >  				   struct nft_regs *regs,
> >  				   const struct nft_pktinfo *pkt)
> >  {
> >  	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> > -	struct nf_conntrack_expect *exp;
> > +	struct nft_ct_expect_data *expect_data;
> >  	enum ip_conntrack_info ctinfo;
> >  	struct nf_conn_help *help;
> > -	enum ip_conntrack_dir dir;
> > -	u16 l3num = priv->l3num;
> >  	struct nf_conn *ct;
> >  
> >  	ct = nf_ct_get(pkt->skb, &ctinfo);
> > @@ -1325,45 +1421,30 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
> >  		regs->verdict.code = NFT_BREAK;
> >  		return;
> >  	}
> > -	dir = CTINFO2DIR(ctinfo);
> >  
> >  	help = nfct_help(ct);
> > -	if (!help)
> > -		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> > -	if (!help) {
> > -		regs->verdict.code = NF_DROP;
> > -		return;
> > -	}
> > -
> > -	if (help->expecting[NF_CT_EXPECT_CLASS_DEFAULT] >= priv->size) {
> > +	if (help) {
> >  		regs->verdict.code = NFT_BREAK;
> >  		return;
> >  	}
> 
> [Severity: Medium]
> Is this change of behaviour intended?  The old code reused an existing
> nf_conn_help extension; now any conntrack that already carries one gives up
> with NFT_BREAK and no expectation is created at all.
> 
> A help extension can already be present on an unconfirmed conntrack from
> several places, for example:
> 
>   - nft_ct_helper_obj_eval() calling nf_ct_helper_ext_add() for a preceding
>     "ct helper set" statement
>   - __nf_ct_try_assign_helper() from init_conntrack()
>   - the exp->assign_helper path in init_conntrack():
> 
> 	assign_helper = rcu_dereference(exp->assign_helper);
> 	if (assign_helper) {
> 		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> 
>   - ctnetlink created conntracks
>   - a second "ct expectation set" statement
> 
> Since NFT_BREAK aborts the whole rule, do the statements following
> "ct expectation set" (counter/log/accept/jump) get skipped too, so such
> packets now fall through to the next rule or the chain policy?
> 
> Also, on the commit message:
> 
>     I'd rather not increase the size of the struct nf_conn for this feature
>     has very limited scope: only one expectation can be created at a time
>     given expect_clash() will make nf_ct_expect_related() reports EBUSY.
> 
> Is that accurate for the pre-patch code?  expect_clash() goes through
> nf_ct_tuple_mask_cmp(), which compares the destination tuple including
> dst.u.all, so two expectation objects with different dports on the same
> master did not clash and could coexist up to priv->size.  Should the commit
> message describe the single-expectation limit as something this patch
> introduces rather than as existing behaviour?
> 
> > -	if (l3num == NFPROTO_INET)
> > -		l3num = nf_ct_l3num(ct);
> >  
> > -	exp = nf_ct_expect_alloc(ct);
> > -	if (exp == NULL) {
> > +	help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> > +	if (!help) {
> >  		regs->verdict.code = NF_DROP;
> >  		return;
> >  	}
> 
> [ ... ]
> 
> > +	expect_data = nfct_help_data(ct);
> > +	if (!expect_data) {
> > +		regs->verdict.code = NFT_BREAK;
> > +		return;
> >  	}
> > -#endif
> > -	if (nf_ct_expect_related(exp, 0) != 0)
> > -		regs->verdict.code = NF_DROP;
> > +	expect_data->obj = *priv;
> > +	expect_data->obj.helper = NULL;
> > +	expect_data->dir = CTINFO2DIR(ctinfo);
> >  
> > -	nf_ct_expect_put(exp);
> > +	if (help && refcount_inc_not_zero(&priv->helper->ct_refcnt))
> > +		rcu_assign_pointer(help->helper, priv->helper);
> >  }