[PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
this helper is going away. Thus, helpers are effectively disabled and no
new expectations are created while removing the expectations created by
this helper as well as unhelping the existing conntrack entries.

Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
- Conntrack confirmation path which invokes the helper callback.
- Propagation of helper to conntrack via expectation.
- OVS ct helper invocation.

Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: use READ_ONCE() and WRITE_ONCE() to modify helper flags are AI reporter suggests.

 include/net/netfilter/nf_conntrack_helper.h | 8 ++++++++
 net/netfilter/nf_conntrack_core.c           | 2 +-
 net/netfilter/nf_conntrack_helper.c         | 5 ++++-
 net/netfilter/nf_conntrack_ovs.c            | 3 +++
 net/netfilter/nf_conntrack_proto.c          | 2 +-
 5 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index de2f956abf34..b6ff7dc65c97 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -25,6 +25,7 @@ struct module;
 enum nf_ct_helper_flags {
 	NF_CT_HELPER_F_USERSPACE	= (1 << 0),
 	NF_CT_HELPER_F_CONFIGURED	= (1 << 1),
+	NF_CT_HELPER_F_DEAD		= (1 << 2),
 };
 
 #define NF_CT_HELPER_NAME_LEN	16
@@ -63,6 +64,13 @@ struct nf_conntrack_helper {
 	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
 };
 
+static inline bool nf_ct_helper_alive(const struct nf_conntrack_helper *helper)
+{
+	unsigned int helper_flags = READ_ONCE(helper->flags);
+
+	return likely(!(helper_flags & NF_CT_HELPER_F_DEAD));
+}
+
 /* Must be kept in sync with the classes defined by helpers */
 #define NF_CT_MAX_EXPECT_CLASSES	4
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 8ba5b22a1eef..d54da6babcfe 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1818,7 +1818,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 			/* exp->master safe, refcnt bumped in nf_ct_find_expectation */
 			ct->master = exp->master;
 			assign_helper = rcu_dereference(exp->assign_helper);
-			if (assign_helper) {
+			if (assign_helper && nf_ct_helper_alive(assign_helper)) {
 				help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 				if (help)
 					rcu_assign_pointer(help->helper, assign_helper);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index b594cd244fe1..9f4ba1b0b5ab 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -415,8 +415,11 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	nf_ct_helper_count--;
 	mutex_unlock(&nf_ct_helper_mutex);
 
+	WRITE_ONCE(me->flags, me->flags | NF_CT_HELPER_F_DEAD);
+
 	/* Make sure every nothing is still using the helper unless its a
-	 * connection in the hash.
+	 * connection in the hash, no more expectations are created after
+	 * this rcu grace period.
 	 */
 	synchronize_rcu();
 
diff --git a/net/netfilter/nf_conntrack_ovs.c b/net/netfilter/nf_conntrack_ovs.c
index a6988eeb1579..eeeb85c18a84 100644
--- a/net/netfilter/nf_conntrack_ovs.c
+++ b/net/netfilter/nf_conntrack_ovs.c
@@ -28,6 +28,9 @@ int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
 	if (!helper)
 		return NF_ACCEPT;
 
+	if (!nf_ct_helper_alive(helper))
+		return NF_ACCEPT;
+
 	if (helper->tuple.src.l3num != NFPROTO_UNSPEC &&
 	    helper->tuple.src.l3num != proto)
 		return NF_ACCEPT;
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 50ddd3d613e1..b2ac5bd491cb 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -174,7 +174,7 @@ unsigned int nf_confirm(void *priv,
 
 		/* rcu_read_lock()ed by nf_hook */
 		helper = rcu_dereference(help->helper);
-		if (helper) {
+		if (helper && nf_ct_helper_alive(helper)) {
 			ret = helper->help(skb,
 					   protoff,
 					   ct, ctinfo);
-- 
2.47.3

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> this helper is going away. Thus, helpers are effectively disabled and no
> new expectations are created while removing the expectations created by
> this helper as well as unhelping the existing conntrack entries.
> 
> Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> - Conntrack confirmation path which invokes the helper callback.
> - Propagation of helper to conntrack via expectation.
> - OVS ct helper invocation.

Not sure this is enough.  New conntracks are not in any hash table /
unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
(can get queued).

> +	WRITE_ONCE(me->flags, me->flags | NF_CT_HELPER_F_DEAD);

How does this avoid race with nfnl_cthelper_update() ?

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

On Thu, May 14, 2026 at 04:43:17PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> > this helper is going away. Thus, helpers are effectively disabled and no
> > new expectations are created while removing the expectations created by
> > this helper as well as unhelping the existing conntrack entries.
> > 
> > Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> > - Conntrack confirmation path which invokes the helper callback.
> > - Propagation of helper to conntrack via expectation.
> > - OVS ct helper invocation.
> 
> Not sure this is enough.  New conntracks are not in any hash table /
> unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
> (can get queued).

nf_ct_iterate_destroy() calls nf_queue_nf_hook_drop() for each netns.

> > +	WRITE_ONCE(me->flags, me->flags | NF_CT_HELPER_F_DEAD);
> 
> How does this avoid race with nfnl_cthelper_update() ?

Hm. I overlook that these flags are toggled, I will propose a new
approach.

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, May 14, 2026 at 04:43:17PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> > > this helper is going away. Thus, helpers are effectively disabled and no
> > > new expectations are created while removing the expectations created by
> > > this helper as well as unhelping the existing conntrack entries.
> > > 
> > > Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> > > - Conntrack confirmation path which invokes the helper callback.
> > > - Propagation of helper to conntrack via expectation.
> > > - OVS ct helper invocation.
> > 
> > Not sure this is enough.  New conntracks are not in any hash table /
> > unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
> > (can get queued).
> 
> nf_ct_iterate_destroy() calls nf_queue_nf_hook_drop() for each netns.

But is that enough?  Consider:

cpu0						cpu1
						recieves verdict
						unlink from nfqueue list
drop_queued_packets (misses unlinked)
						... going on ..

I think to properly resolve this, there is a need to check
for this new dead flag after queueing to userspace (after its on list)
and again when receiving the verdict.

Arguably this is kind of different bug, because this comment is wrong:

/* a skb w. unconfirmed conntrack could have been reinjected just
 * before we called nf_queue_nf_hook_drop().
 *
 * This makes sure its inserted into conntrack table.
 */
 synchronize_net();

(it could have been requeued).

I think a more generic fix is to add a seqcnt to nf_queue_entry.
When queueing, record current seqcnt.

On reinject, drop if unconfirmed and seqcnt_now != entry->seqcnt.
Not nice, but I don't see a better way ATM.

The seqcnt can be pernet and it can be restricted to nfnetlink_queue.

Any better idea?

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

On Thu, May 14, 2026 at 05:44:58PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, May 14, 2026 at 04:43:17PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> > > > this helper is going away. Thus, helpers are effectively disabled and no
> > > > new expectations are created while removing the expectations created by
> > > > this helper as well as unhelping the existing conntrack entries.
> > > > 
> > > > Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> > > > - Conntrack confirmation path which invokes the helper callback.
> > > > - Propagation of helper to conntrack via expectation.
> > > > - OVS ct helper invocation.
> > > 
> > > Not sure this is enough.  New conntracks are not in any hash table /
> > > unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
> > > (can get queued).
> > 
> > nf_ct_iterate_destroy() calls nf_queue_nf_hook_drop() for each netns.
> 
> But is that enough?  Consider:
> 
> cpu0						cpu1
> 						recieves verdict
> 						unlink from nfqueue list
> drop_queued_packets (misses unlinked)
> 						... going on ..

This looks like a general problem with nf_queue_nf_hook_drop().

> I think to properly resolve this, there is a need to check
> for this new dead flag after queueing to userspace (after its on list)
> and again when receiving the verdict.
>
> Arguably this is kind of different bug, because this comment is wrong:
> 
> /* a skb w. unconfirmed conntrack could have been reinjected just
>  * before we called nf_queue_nf_hook_drop().
>  *
>  * This makes sure its inserted into conntrack table.
>  */
>  synchronize_net();
> 
> (it could have been requeued).
>
> I think a more generic fix is to add a seqcnt to nf_queue_entry.
> When queueing, record current seqcnt.
>
> On reinject, drop if unconfirmed and seqcnt_now != entry->seqcnt.
> Not nice, but I don't see a better way ATM.

But you would need to check right before enqueueing (adding to the
hashtable/list), so the race would still be there? 

> The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> 
> Any better idea?

Maybe add a helper_id which is set at helper registration time. Then
nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
reinject use this helper_id to re-lookup the helper when reinjecting.
This would force a slow path for unconfirmed conntracks, to
re-validate if the helper is still there.

cttimeout would need this too, a lookup to check if the timeout policy
is still around.

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

On Fri, May 15, 2026 at 01:30:33AM +0200, Pablo Neira Ayuso wrote:
> On Thu, May 14, 2026 at 05:44:58PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Thu, May 14, 2026 at 04:43:17PM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> > > > > this helper is going away. Thus, helpers are effectively disabled and no
> > > > > new expectations are created while removing the expectations created by
> > > > > this helper as well as unhelping the existing conntrack entries.
> > > > > 
> > > > > Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> > > > > - Conntrack confirmation path which invokes the helper callback.
> > > > > - Propagation of helper to conntrack via expectation.
> > > > > - OVS ct helper invocation.
> > > > 
> > > > Not sure this is enough.  New conntracks are not in any hash table /
> > > > unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
> > > > (can get queued).
> > > 
> > > nf_ct_iterate_destroy() calls nf_queue_nf_hook_drop() for each netns.
> > 
> > But is that enough?  Consider:
> > 
> > cpu0						cpu1
> > 						recieves verdict
> > 						unlink from nfqueue list
> > drop_queued_packets (misses unlinked)
> > 						... going on ..
> 
> This looks like a general problem with nf_queue_nf_hook_drop().
> 
> > I think to properly resolve this, there is a need to check
> > for this new dead flag after queueing to userspace (after its on list)
> > and again when receiving the verdict.
> >
> > Arguably this is kind of different bug, because this comment is wrong:
> > 
> > /* a skb w. unconfirmed conntrack could have been reinjected just
> >  * before we called nf_queue_nf_hook_drop().
> >  *
> >  * This makes sure its inserted into conntrack table.
> >  */
> >  synchronize_net();
> > 
> > (it could have been requeued).
> >
> > I think a more generic fix is to add a seqcnt to nf_queue_entry.
> > When queueing, record current seqcnt.
> >
> > On reinject, drop if unconfirmed and seqcnt_now != entry->seqcnt.
> > Not nice, but I don't see a better way ATM.
> 
> But you would need to check right before enqueueing (adding to the
> hashtable/list), so the race would still be there? 
> 
> > The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> > 
> > Any better idea?
> 
> Maybe add a helper_id which is set at helper registration time. Then
> nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
> reinject use this helper_id to re-lookup the helper when reinjecting.
> This would force a slow path for unconfirmed conntracks, to
> re-validate if the helper is still there.
> 
> cttimeout would need this too, a lookup to check if the timeout policy
> is still around.

Hm.

struct nf_ct_ext {
        u8 offset[NF_CT_EXT_NUM];
        u8 len;
        unsigned int gen_id;   <---- There is already a gen_id here.

And nf_ct_ext_bump_genid() is called from nf_ct_iterate_destroy().

Maybe we could simply check if there is a mismatch in this generation
id from reinject path?

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> > 
> > Any better idea?
> 
> Maybe add a helper_id which is set at helper registration time. Then
> nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
> reinject use this helper_id to re-lookup the helper when reinjecting.
> This would force a slow path for unconfirmed conntracks, to
> re-validate if the helper is still there.
> 
> cttimeout would need this too, a lookup to check if the timeout policy
> is still around.

Hmm, maybe just re-use the nf_conntrack_ext_genid for this?
I think this unreg/rmmod isn't so frequent.

Another alternative would be to give up on this design completely
and just grab module references :-)

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

On Fri, May 15, 2026 at 01:55:08AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> > > 
> > > Any better idea?
> > 
> > Maybe add a helper_id which is set at helper registration time. Then
> > nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
> > reinject use this helper_id to re-lookup the helper when reinjecting.
> > This would force a slow path for unconfirmed conntracks, to
> > re-validate if the helper is still there.
> > 
> > cttimeout would need this too, a lookup to check if the timeout policy
> > is still around.
> 
> Hmm, maybe just re-use the nf_conntrack_ext_genid for this?
> I think this unreg/rmmod isn't so frequent.

nf_ct_iterate_destroy() is called for both cthelper/cttimeout, which
already bumps nf_conntrack_ext_genid.

Simply add the check from nf_reinject() path then?

> Another alternative would be to give up on this design completely
> and just grab module references :-)

But that would not be enough for userspace ct helpers, right?

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

On Fri, May 15, 2026 at 02:10:53AM +0200, Pablo Neira Ayuso wrote:
> On Fri, May 15, 2026 at 01:55:08AM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> > > > 
> > > > Any better idea?
> > > 
> > > Maybe add a helper_id which is set at helper registration time. Then
> > > nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
> > > reinject use this helper_id to re-lookup the helper when reinjecting.
> > > This would force a slow path for unconfirmed conntracks, to
> > > re-validate if the helper is still there.
> > > 
> > > cttimeout would need this too, a lookup to check if the timeout policy
> > > is still around.
> > 
> > Hmm, maybe just re-use the nf_conntrack_ext_genid for this?
> > I think this unreg/rmmod isn't so frequent.
> 
> nf_ct_iterate_destroy() is called for both cthelper/cttimeout, which
> already bumps nf_conntrack_ext_genid.
> 
> Simply add the check from nf_reinject() path then?

If the module reference grab does not work, maybe add:
 
        if (unlikely(nf_conntrack_ext_genid() != ext->id)
                return NULL;
 
to nfct_help() and nfct_timeout()? So access to these ct extension
area is always validated before hand?

> > Another alternative would be to give up on this design completely
> > and just grab module references :-)
> 
> But that would not be enough for userspace ct helpers, right?

Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> If the module reference grab does not work, maybe add:
>  
>         if (unlikely(nf_conntrack_ext_genid() != ext->id)
>                 return NULL;
>  
> to nfct_help() and nfct_timeout()? So access to these ct extension
> area is always validated before hand?
> 
> > > Another alternative would be to give up on this design completely
> > > and just grab module references :-)
> > 
> > But that would not be enough for userspace ct helpers, right?

This is a mess:

https://sashiko.dev/#/patchset/20260515103501.18669-1-fw%40strlen.de

No idea how to fix this yet.  Is it ok to disable cross-helper-attach
via ctnetlink?  I don't see a way to validate nfct_help_data().

Proposal: Get rid of data[] and nfct_help_data().  Explicit structure,
explicit helpers (e.g. nfct_help_data_sip(), type enum in nf_conn_help.

Callers must handle NULL return value everywhere (wrong helper type,
helper invalidated, etc).

unhelp will have to be changed to invoke the helper
destructor as well:

static int unhelp(struct nf_conn *ct, void *me)
{
        struct nf_conn_help *help = nfct_help(ct);

        if (help && rcu_dereference_raw(help->helper) == me) {
                nf_conntrack_event(IPCT_HELPER, ct);
                RCU_INIT_POINTER(help->helper, NULL);
        }

This can't be right, we lose the ->destroy() CB?

Ideally we could get rid of ->destroy, but that would require
permanent removal of pptp.