* [PATCH nf-next 1/6] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 2/6] netfilter: cttimeout: detach dataplane timeout policy and add refcount Pablo Neira Ayuso
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
Conntrack helper flags are accessed from packet and netlink dump path.
Concurrent update of userspace helper flags is not possible, because the
nfnl_mutex in held on updates. These flags are only used by userspace
helpers. Use {READ,WRITE}_ONCE() to access this flags from lockless
paths.
Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_core.c | 4 +++-
net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++++-------
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 8ba5b22a1eef..60973ba58820 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2206,6 +2206,7 @@ static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
{
const struct nf_conntrack_helper *helper;
const struct nf_conn_help *help;
+ unsigned int helper_flags;
int protoff;
help = nfct_help(ct);
@@ -2216,7 +2217,8 @@ static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
if (!helper)
return NF_ACCEPT;
- if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
+ helper_flags = READ_ONCE(helper->flags);
+ if (!(helper_flags & NF_CT_HELPER_F_USERSPACE))
return NF_ACCEPT;
switch (nf_ct_l3num(ct)) {
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 0d16ad82d70c..61a2407b53bd 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -41,8 +41,9 @@ static int
nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
struct nf_conn *ct, enum ip_conntrack_info ctinfo)
{
- const struct nf_conn_help *help;
struct nf_conntrack_helper *helper;
+ const struct nf_conn_help *help;
+ unsigned int helper_flags;
help = nfct_help(ct);
if (help == NULL)
@@ -53,8 +54,10 @@ nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
if (helper == NULL)
return NF_DROP;
+ helper_flags = READ_ONCE(helper->flags);
+
/* This is a user-space helper not yet configured, skip. */
- if ((helper->flags &
+ if ((helper_flags &
(NF_CT_HELPER_F_USERSPACE | NF_CT_HELPER_F_CONFIGURED)) ==
NF_CT_HELPER_F_USERSPACE)
return NF_ACCEPT;
@@ -404,10 +407,10 @@ nfnl_cthelper_update(const struct nlattr * const tb[],
switch(status) {
case NFCT_HELPER_STATUS_ENABLED:
- helper->flags |= NF_CT_HELPER_F_CONFIGURED;
+ WRITE_ONCE(helper->flags, helper->flags | NF_CT_HELPER_F_CONFIGURED);
break;
case NFCT_HELPER_STATUS_DISABLED:
- helper->flags &= ~NF_CT_HELPER_F_CONFIGURED;
+ WRITE_ONCE(helper->flags, helper->flags & ~NF_CT_HELPER_F_CONFIGURED);
break;
}
}
@@ -529,8 +532,8 @@ static int
nfnl_cthelper_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
int event, struct nf_conntrack_helper *helper)
{
- struct nlmsghdr *nlh;
unsigned int flags = portid ? NLM_F_MULTI : 0;
+ struct nlmsghdr *nlh;
int status;
event = nfnl_msg_type(NFNL_SUBSYS_CTHELPER, event);
@@ -554,7 +557,7 @@ nfnl_cthelper_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
if (nla_put_be32(skb, NFCTH_PRIV_DATA_LEN, htonl(helper->data_len)))
goto nla_put_failure;
- if (helper->flags & NF_CT_HELPER_F_CONFIGURED)
+ if (READ_ONCE(helper->flags) & NF_CT_HELPER_F_CONFIGURED)
status = NFCT_HELPER_STATUS_ENABLED;
else
status = NFCT_HELPER_STATUS_DISABLED;
@@ -575,6 +578,7 @@ static int
nfnl_cthelper_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
{
struct nf_conntrack_helper *cur, *last;
+ unsigned int helper_flags;
rcu_read_lock();
last = (struct nf_conntrack_helper *)cb->args[1];
@@ -583,8 +587,10 @@ nfnl_cthelper_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
hlist_for_each_entry_rcu(cur,
&nf_ct_helper_hash[cb->args[0]], hnode) {
+ helper_flags = READ_ONCE(cur->flags);
+
/* skip non-userspace conntrack helpers. */
- if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
+ if (!(helper_flags & NF_CT_HELPER_F_USERSPACE))
continue;
if (cb->args[1]) {
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH nf-next 2/6] netfilter: cttimeout: detach dataplane timeout policy and add refcount
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 1/6] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 3/6] netfilter: nf_conntrack_helper: dynamically allocate struct nf_conntrack_helper Pablo Neira Ayuso
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
Add a refcount for struct nf_ct_timeout which is used by ct extension to
set the custom ct timeout policy, this tells us the ct timeout is being
used by a conntrack entry.
There is already a refcount for control plane which controls if the
ruleset refers to the timeout policy. This patch still allows users to
remove the ct timeout policy from control plane if it has no more users
in the ruleset, but the ct timeout object remains in memory if it has
conntrack entries that still use them. When the last conntrack entry
drops the refcount on the ct timeout, the ct timeout is released.
The inner nf_ct_timeout holds an initial reference on behalf of the
ctnl_timeout wrapper; per-conntrack references are taken on top of that via
nf_ct_timeout_ext_add().
Remove nf_queue_nf_hook_drop(): a packet sitting in nfqueue will just
hold a reference to the nf_ct_timeout object until packet is reinjected,
since this is part of the ct extension, this will be released by the
time the conntrack is freed.
nf_ct_untimeout() is still called to clean up in a best effort basis:
the ct timeout on existing entries gets remove when the ct timeout goes
away, but racing new unconfirmed conntrack entries could still attach
it, postponing release after that user of it is gone.
Fixes: 50978462300f ("netfilter: add cttimeout infrastructure for fine timeout tuning")
Fixes: 7e0b2b57f01d ("netfilter: nft_ct: add ct timeout support")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack.h | 2 +
include/net/netfilter/nf_conntrack_timeout.h | 21 ++++++
net/netfilter/nf_conntrack_core.c | 11 +--
net/netfilter/nf_conntrack_timeout.c | 20 +++++-
net/netfilter/nfnetlink_cttimeout.c | 74 ++++++++++++--------
net/netfilter/nft_ct.c | 5 +-
net/netfilter/xt_CT.c | 2 +-
7 files changed, 94 insertions(+), 41 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index bc42dd0e10e6..f75af8eb1cae 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -239,6 +239,8 @@ struct nf_ct_iter_data {
};
/* Iterate over all conntracks: if iter returns true, it's deleted. */
+void nf_ct_iterate_cleanup(int (*iter)(struct nf_conn *i, void *data),
+ const struct nf_ct_iter_data *iter_data);
void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
const struct nf_ct_iter_data *iter_data);
diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
index 3a66d4abb6d6..7659e8a1abd9 100644
--- a/include/net/netfilter/nf_conntrack_timeout.h
+++ b/include/net/netfilter/nf_conntrack_timeout.h
@@ -12,6 +12,8 @@
#define CTNL_TIMEOUT_NAME_MAX 32
struct nf_ct_timeout {
+ refcount_t refcnt;
+ struct ctnl_timeout *ctnl; /* for nfnetlink_cttimeout. */
__u16 l3num;
const struct nf_conntrack_l4proto *l4proto;
struct rcu_head rcu;
@@ -22,6 +24,22 @@ struct nf_conn_timeout {
struct nf_ct_timeout __rcu *timeout;
};
+static inline void nf_ct_timeout_put(const struct nf_conn *ct)
+{
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+ struct nf_conn_timeout *timeout_ext;
+ struct nf_ct_timeout *timeout;
+
+ timeout_ext = nf_ct_ext_find(ct, NF_CT_EXT_TIMEOUT);
+ if (!timeout_ext)
+ return;
+
+ timeout = rcu_dereference(timeout_ext->timeout);
+ if (likely(timeout) && refcount_dec_and_test(&timeout->refcnt))
+ kfree_rcu(timeout, rcu);
+#endif
+}
+
static inline unsigned int *
nf_ct_timeout_data(const struct nf_conn_timeout *t)
{
@@ -60,6 +78,9 @@ struct nf_conn_timeout *nf_ct_timeout_ext_add(struct nf_conn *ct,
if (timeout_ext == NULL)
return NULL;
+ if (!refcount_inc_not_zero(&timeout->refcnt))
+ return NULL;
+
rcu_assign_pointer(timeout_ext->timeout, timeout);
return timeout_ext;
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 60973ba58820..63159c070c3a 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1730,16 +1730,18 @@ void nf_conntrack_free(struct nf_conn *ct)
*/
WARN_ON(refcount_read(&ct->ct_general.use) != 0);
+ rcu_read_lock();
if (ct->status & IPS_SRC_NAT_DONE) {
const struct nf_nat_hook *nat_hook;
- rcu_read_lock();
nat_hook = rcu_dereference(nf_nat_hook);
if (nat_hook)
nat_hook->remove_nat_bysrc(ct);
- rcu_read_unlock();
}
+ nf_ct_timeout_put(ct);
+ rcu_read_unlock();
+
kfree(ct->ext);
kmem_cache_free(nf_conntrack_cachep, ct);
cnet = nf_ct_pernet(net);
@@ -2356,8 +2358,8 @@ get_next_corpse(int (*iter)(struct nf_conn *i, void *data),
return ct;
}
-static void nf_ct_iterate_cleanup(int (*iter)(struct nf_conn *i, void *data),
- const struct nf_ct_iter_data *iter_data)
+void nf_ct_iterate_cleanup(int (*iter)(struct nf_conn *i, void *data),
+ const struct nf_ct_iter_data *iter_data)
{
unsigned int bucket = 0;
struct nf_conn *ct;
@@ -2374,6 +2376,7 @@ static void nf_ct_iterate_cleanup(int (*iter)(struct nf_conn *i, void *data),
}
mutex_unlock(&nf_conntrack_mutex);
}
+EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup);
void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
const struct nf_ct_iter_data *iter_data)
diff --git a/net/netfilter/nf_conntrack_timeout.c b/net/netfilter/nf_conntrack_timeout.c
index 0cc584d3dbb1..00281db8e410 100644
--- a/net/netfilter/nf_conntrack_timeout.c
+++ b/net/netfilter/nf_conntrack_timeout.c
@@ -25,17 +25,28 @@
const struct nf_ct_timeout_hooks __rcu *nf_ct_timeout_hook __read_mostly;
EXPORT_SYMBOL_GPL(nf_ct_timeout_hook);
+/* nf_ct_iterate_cleanup() holds refcount on this conntrack. */
static int untimeout(struct nf_conn *ct, void *timeout)
{
struct nf_conn_timeout *timeout_ext = nf_ct_timeout_find(ct);
if (timeout_ext) {
- const struct nf_ct_timeout *t;
+ struct nf_ct_timeout *t;
t = rcu_access_pointer(timeout_ext->timeout);
+ if (!t)
+ return 0;
- if (!timeout || t == timeout)
+ if (!timeout || t == timeout) {
RCU_INIT_POINTER(timeout_ext->timeout, NULL);
+
+ /* No race with nf_conntrack_free() which is called
+ * only after the conntrack has been removed from
+ * the hashes.
+ */
+ if (refcount_dec_and_test(&t->refcnt))
+ kfree_rcu(t, rcu);
+ }
}
/* We are not intended to delete this conntrack. */
@@ -49,7 +60,10 @@ void nf_ct_untimeout(struct net *net, struct nf_ct_timeout *timeout)
.data = timeout,
};
- nf_ct_iterate_cleanup_net(untimeout, &iter_data);
+ if (net)
+ nf_ct_iterate_cleanup_net(untimeout, &iter_data);
+ else
+ nf_ct_iterate_cleanup(untimeout, &iter_data);
}
EXPORT_SYMBOL_GPL(nf_ct_untimeout);
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index dca6826af7de..8efda53f94eb 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -39,9 +39,7 @@ struct ctnl_timeout {
struct rcu_head rcu_head;
refcount_t refcnt;
char name[CTNL_TIMEOUT_NAME_MAX];
-
- /* must be at the end */
- struct nf_ct_timeout timeout;
+ struct nf_ct_timeout *timeout;
};
struct nfct_timeout_pernet {
@@ -132,12 +130,12 @@ static int cttimeout_new_timeout(struct sk_buff *skb,
/* You cannot replace one timeout policy by another of
* different kind, sorry.
*/
- if (matching->timeout.l3num != l3num ||
- matching->timeout.l4proto->l4proto != l4num)
+ if (matching->timeout->l3num != l3num ||
+ matching->timeout->l4proto->l4proto != l4num)
return -EINVAL;
- return ctnl_timeout_parse_policy(&matching->timeout.data,
- matching->timeout.l4proto,
+ return ctnl_timeout_parse_policy(&matching->timeout->data,
+ matching->timeout->l4proto,
info->net,
cda[CTA_TIMEOUT_DATA]);
}
@@ -153,26 +151,37 @@ static int cttimeout_new_timeout(struct sk_buff *skb,
goto err_proto_put;
}
- timeout = kzalloc(sizeof(struct ctnl_timeout) +
- l4proto->ctnl_timeout.obj_size, GFP_KERNEL);
+ timeout = kzalloc(sizeof(*timeout), GFP_KERNEL);
if (timeout == NULL) {
ret = -ENOMEM;
goto err_proto_put;
}
- ret = ctnl_timeout_parse_policy(&timeout->timeout.data, l4proto,
+ timeout->timeout = kzalloc(sizeof(*timeout->timeout) +
+ l4proto->ctnl_timeout.obj_size, GFP_KERNEL);
+ if (!timeout->timeout) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = ctnl_timeout_parse_policy(&timeout->timeout->data, l4proto,
info->net, cda[CTA_TIMEOUT_DATA]);
if (ret < 0)
- goto err;
+ goto err_parse_timeout_policy;
strcpy(timeout->name, nla_data(cda[CTA_TIMEOUT_NAME]));
- timeout->timeout.l3num = l3num;
- timeout->timeout.l4proto = l4proto;
refcount_set(&timeout->refcnt, 1);
+ timeout->timeout->ctnl = timeout;
+ timeout->timeout->l3num = l3num;
+ timeout->timeout->l4proto = l4proto;
+ refcount_set(&timeout->timeout->refcnt, 1);
__module_get(THIS_MODULE);
list_add_tail_rcu(&timeout->head, &pernet->nfct_timeout_list);
return 0;
+
+err_parse_timeout_policy:
+ kfree(timeout->timeout);
err:
kfree(timeout);
err_proto_put:
@@ -185,7 +194,7 @@ ctnl_timeout_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
{
struct nlmsghdr *nlh;
unsigned int flags = portid ? NLM_F_MULTI : 0;
- const struct nf_conntrack_l4proto *l4proto = timeout->timeout.l4proto;
+ const struct nf_conntrack_l4proto *l4proto = timeout->timeout->l4proto;
struct nlattr *nest_parms;
int ret;
@@ -197,7 +206,7 @@ ctnl_timeout_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
if (nla_put_string(skb, CTA_TIMEOUT_NAME, timeout->name) ||
nla_put_be16(skb, CTA_TIMEOUT_L3PROTO,
- htons(timeout->timeout.l3num)) ||
+ htons(timeout->timeout->l3num)) ||
nla_put_u8(skb, CTA_TIMEOUT_L4PROTO, l4proto->l4proto) ||
nla_put_be32(skb, CTA_TIMEOUT_USE,
htonl(refcount_read(&timeout->refcnt))))
@@ -207,7 +216,7 @@ ctnl_timeout_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
if (!nest_parms)
goto nla_put_failure;
- ret = l4proto->ctnl_timeout.obj_to_nlattr(skb, &timeout->timeout.data);
+ ret = l4proto->ctnl_timeout.obj_to_nlattr(skb, &timeout->timeout->data);
if (ret < 0)
goto nla_put_failure;
@@ -316,9 +325,20 @@ static int ctnl_timeout_try_del(struct net *net, struct ctnl_timeout *timeout)
* current refcnt is 1, we decrease it to 0.
*/
if (refcount_dec_if_one(&timeout->refcnt)) {
+ /* ->timeout_put is called by template conntrack in xt_CT and
+ * OVS to drop the reference on this timeout policy. This can
+ * only be 1 if this timeout policy unused. It is safe to
+ * reset this ->ctnl indirection here because it has no users.
+ */
+ WRITE_ONCE(timeout->timeout->ctnl, NULL);
+
/* We are protected by nfnl mutex. */
list_del_rcu(&timeout->head);
- nf_ct_untimeout(net, &timeout->timeout);
+ nf_ct_untimeout(net, timeout->timeout);
+
+ if (refcount_dec_and_test(&timeout->timeout->refcnt))
+ kfree_rcu(timeout->timeout, rcu);
+
kfree_rcu(timeout, rcu_head);
} else {
ret = -EBUSY;
@@ -517,13 +537,15 @@ static struct nf_ct_timeout *ctnl_timeout_find_get(struct net *net,
break;
}
err:
- return matching ? &matching->timeout : NULL;
+ return matching ? matching->timeout : NULL;
}
static void ctnl_timeout_put(struct nf_ct_timeout *t)
{
- struct ctnl_timeout *timeout =
- container_of(t, struct ctnl_timeout, timeout);
+ struct ctnl_timeout *timeout = READ_ONCE(t->ctnl);
+
+ if (!timeout)
+ return;
if (refcount_dec_and_test(&timeout->refcnt)) {
kfree_rcu(timeout, rcu_head);
@@ -649,16 +671,6 @@ static int __init cttimeout_init(void)
return ret;
}
-static int untimeout(struct nf_conn *ct, void *timeout)
-{
- struct nf_conn_timeout *timeout_ext = nf_ct_timeout_find(ct);
-
- if (timeout_ext)
- RCU_INIT_POINTER(timeout_ext->timeout, NULL);
-
- return 0;
-}
-
static void __exit cttimeout_exit(void)
{
nfnetlink_subsys_unregister(&cttimeout_subsys);
@@ -666,7 +678,7 @@ static void __exit cttimeout_exit(void)
unregister_pernet_subsys(&cttimeout_ops);
RCU_INIT_POINTER(nf_ct_timeout_hook, NULL);
- nf_ct_iterate_destroy(untimeout, NULL);
+ nf_ct_untimeout(NULL, NULL);
}
module_init(cttimeout_init);
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index fa2cc556331c..85e3d68dfb59 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -951,6 +951,7 @@ static int nft_ct_timeout_obj_init(const struct nft_ctx *ctx,
timeout->l3num = l3num;
timeout->l4proto = l4proto;
+ refcount_set(&timeout->refcnt, 1);
ret = nf_ct_netns_get(ctx->net, ctx->family);
if (ret < 0)
@@ -971,10 +972,10 @@ static void nft_ct_timeout_obj_destroy(const struct nft_ctx *ctx,
struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
struct nf_ct_timeout *timeout = priv->timeout;
- nf_queue_nf_hook_drop(ctx->net);
nf_ct_untimeout(ctx->net, timeout);
nf_ct_netns_put(ctx->net, ctx->family);
- kfree_rcu(priv->timeout, rcu);
+ if (refcount_dec_and_test(&timeout->refcnt))
+ kfree_rcu(priv->timeout, rcu);
}
static int nft_ct_timeout_obj_dump(struct sk_buff *skb,
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index d2aeacf94230..b94f004d5f5c 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -284,7 +284,7 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param *par,
struct nf_conn_help *help;
if (ct) {
- if (info->helper[0] || info->timeout[0])
+ if (info->helper[0])
nf_queue_nf_hook_drop(par->net);
help = nfct_help(ct);
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH nf-next 3/6] netfilter: nf_conntrack_helper: dynamically allocate struct nf_conntrack_helper
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 1/6] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 2/6] netfilter: cttimeout: detach dataplane timeout policy and add refcount Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 4/6] netfilter: nf_conntrack_pptp: move GRE specific cleanup to GRE tracker Pablo Neira Ayuso
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
Adapt all existing helpers to use a modified version of
nf_ct_helper_init(), to dynamically allocate struct nf_conntrack_helper.
Allocate expect_policy[] built-in into the helper to ensure this area is
reachable after helper removal since a follow up patch adds refcount to
track use of the nf_conntrack_helper structure from packet path so it
remains around until last reference from ct helper extension is dropped.
Export __nf_conntrack_helper_register() which allows to register
nfnetlink_cthelper dynamically allocated helper. Adapt nfnetlink_cthelper
to use the built-in expect_policy[].
This is a preparation patch to add packet path refcounting to helpers.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack_helper.h | 16 ++--
net/ipv4/netfilter/nf_nat_snmp_basic_main.c | 19 ++---
net/netfilter/nf_conntrack_amanda.c | 39 ++++-----
net/netfilter/nf_conntrack_ftp.c | 5 +-
net/netfilter/nf_conntrack_h323_main.c | 91 ++++++++-------------
net/netfilter/nf_conntrack_helper.c | 71 +++++++++++++---
net/netfilter/nf_conntrack_irc.c | 5 +-
net/netfilter/nf_conntrack_netbios_ns.c | 18 ++--
net/netfilter/nf_conntrack_pptp.c | 26 +++---
net/netfilter/nf_conntrack_sane.c | 5 +-
net/netfilter/nf_conntrack_sip.c | 5 +-
net/netfilter/nf_conntrack_snmp.c | 21 +++--
net/netfilter/nf_conntrack_tftp.c | 5 +-
net/netfilter/nfnetlink_cthelper.c | 20 +----
14 files changed, 174 insertions(+), 172 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index de2f956abf34..1956bc12bf56 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -29,13 +29,16 @@ enum nf_ct_helper_flags {
#define NF_CT_HELPER_NAME_LEN 16
+/* Must be kept in sync with the classes defined by helpers */
+#define NF_CT_MAX_EXPECT_CLASSES 4
+
struct nf_conntrack_helper {
struct hlist_node hnode; /* Internal use. */
char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
refcount_t refcnt;
struct module *me; /* pointer to self */
- const struct nf_conntrack_expect_policy *expect_policy;
+ struct nf_conntrack_expect_policy expect_policy[NF_CT_MAX_EXPECT_CLASSES];
/* Tuple of things we will help (compared against server response) */
struct nf_conntrack_tuple tuple;
@@ -63,9 +66,6 @@ struct nf_conntrack_helper {
char nat_mod_name[NF_CT_HELPER_NAME_LEN];
};
-/* Must be kept in sync with the classes defined by helpers */
-#define NF_CT_MAX_EXPECT_CLASSES 4
-
/* nf_conn feature for connections that have a helper */
struct nf_conn_help {
/* Helper. if any */
@@ -103,11 +103,13 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
struct nf_conn *ct),
struct module *module);
-int nf_conntrack_helper_register(struct nf_conntrack_helper *);
+int nf_conntrack_helper_register(struct nf_conntrack_helper *, struct nf_conntrack_helper **);
+int __nf_conntrack_helper_register(struct nf_conntrack_helper *);
void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
-int nf_conntrack_helpers_register(struct nf_conntrack_helper *, unsigned int);
-void nf_conntrack_helpers_unregister(struct nf_conntrack_helper *,
+int nf_conntrack_helpers_register(struct nf_conntrack_helper *, unsigned int,
+ struct nf_conntrack_helper **);
+void nf_conntrack_helpers_unregister(struct nf_conntrack_helper **,
unsigned int);
struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
diff --git a/net/ipv4/netfilter/nf_nat_snmp_basic_main.c b/net/ipv4/netfilter/nf_nat_snmp_basic_main.c
index 717b726504fe..27ebc0a6efcd 100644
--- a/net/ipv4/netfilter/nf_nat_snmp_basic_main.c
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic_main.c
@@ -202,29 +202,26 @@ static const struct nf_conntrack_expect_policy snmp_exp_policy = {
.timeout = 180,
};
-static struct nf_conntrack_helper snmp_trap_helper __read_mostly = {
- .me = THIS_MODULE,
- .help = help,
- .expect_policy = &snmp_exp_policy,
- .name = "snmp_trap",
- .tuple.src.l3num = AF_INET,
- .tuple.src.u.udp.port = cpu_to_be16(SNMP_TRAP_PORT),
- .tuple.dst.protonum = IPPROTO_UDP,
-};
+static struct nf_conntrack_helper snmp_trap_helper __read_mostly;
+static struct nf_conntrack_helper *snmp_trap_helper_ptr __read_mostly;
static int __init nf_nat_snmp_basic_init(void)
{
BUG_ON(nf_nat_snmp_hook != NULL);
RCU_INIT_POINTER(nf_nat_snmp_hook, help);
- return nf_conntrack_helper_register(&snmp_trap_helper);
+ nf_ct_helper_init(&snmp_trap_helper, AF_INET, IPPROTO_UDP,
+ "snmp_trap", SNMP_TRAP_PORT, SNMP_TRAP_PORT, SNMP_TRAP_PORT,
+ &snmp_exp_policy, 0, help, NULL, THIS_MODULE);
+
+ return nf_conntrack_helper_register(&snmp_trap_helper, &snmp_trap_helper_ptr);
}
static void __exit nf_nat_snmp_basic_fini(void)
{
RCU_INIT_POINTER(nf_nat_snmp_hook, NULL);
synchronize_rcu();
- nf_conntrack_helper_unregister(&snmp_trap_helper);
+ nf_conntrack_helper_unregister(snmp_trap_helper_ptr);
}
module_init(nf_nat_snmp_basic_init);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index d2c09e8dd872..ddafbdfc96dc 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -169,35 +169,15 @@ static const struct nf_conntrack_expect_policy amanda_exp_policy = {
.timeout = 180,
};
-static struct nf_conntrack_helper amanda_helper[2] __read_mostly = {
- {
- .name = HELPER_NAME,
- .me = THIS_MODULE,
- .help = amanda_help,
- .tuple.src.l3num = AF_INET,
- .tuple.src.u.udp.port = cpu_to_be16(10080),
- .tuple.dst.protonum = IPPROTO_UDP,
- .expect_policy = &amanda_exp_policy,
- .nat_mod_name = NF_NAT_HELPER_NAME(HELPER_NAME),
- },
- {
- .name = "amanda",
- .me = THIS_MODULE,
- .help = amanda_help,
- .tuple.src.l3num = AF_INET6,
- .tuple.src.u.udp.port = cpu_to_be16(10080),
- .tuple.dst.protonum = IPPROTO_UDP,
- .expect_policy = &amanda_exp_policy,
- .nat_mod_name = NF_NAT_HELPER_NAME(HELPER_NAME),
- },
-};
+static struct nf_conntrack_helper amanda_helper[2] __read_mostly;
+static struct nf_conntrack_helper *amanda_helper_ptr[2] __read_mostly;
static void __exit nf_conntrack_amanda_fini(void)
{
int i;
- nf_conntrack_helpers_unregister(amanda_helper,
- ARRAY_SIZE(amanda_helper));
+ nf_conntrack_helpers_unregister(amanda_helper_ptr,
+ ARRAY_SIZE(amanda_helper_ptr));
for (i = 0; i < ARRAY_SIZE(search); i++)
textsearch_destroy(search[i].ts);
}
@@ -217,8 +197,17 @@ static int __init nf_conntrack_amanda_init(void)
goto err1;
}
}
+
+ nf_ct_helper_init(&amanda_helper[0], AF_INET, IPPROTO_UDP,
+ HELPER_NAME, 10080, 10080, 10080,
+ &amanda_exp_policy, 0, amanda_help, NULL, THIS_MODULE);
+ nf_ct_helper_init(&amanda_helper[1], AF_INET6, IPPROTO_UDP,
+ HELPER_NAME, 10080, 10080, 10080,
+ &amanda_exp_policy, 0, amanda_help, NULL, THIS_MODULE);
+
ret = nf_conntrack_helpers_register(amanda_helper,
- ARRAY_SIZE(amanda_helper));
+ ARRAY_SIZE(amanda_helper),
+ amanda_helper_ptr);
if (ret < 0)
goto err1;
return 0;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index de83bf9e6c61..b21da0c78845 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -552,6 +552,7 @@ static int nf_ct_ftp_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
}
static struct nf_conntrack_helper ftp[MAX_PORTS * 2] __read_mostly;
+static struct nf_conntrack_helper *ftp_ptr[MAX_PORTS * 2] __read_mostly;
static const struct nf_conntrack_expect_policy ftp_exp_policy = {
.max_expected = 1,
@@ -560,7 +561,7 @@ static const struct nf_conntrack_expect_policy ftp_exp_policy = {
static void __exit nf_conntrack_ftp_fini(void)
{
- nf_conntrack_helpers_unregister(ftp, ports_c * 2);
+ nf_conntrack_helpers_unregister(ftp_ptr, ports_c * 2);
}
static int __init nf_conntrack_ftp_init(void)
@@ -585,7 +586,7 @@ static int __init nf_conntrack_ftp_init(void)
nf_ct_ftp_from_nlattr, THIS_MODULE);
}
- ret = nf_conntrack_helpers_register(ftp, ports_c * 2);
+ ret = nf_conntrack_helpers_register(ftp, ports_c * 2, ftp_ptr);
if (ret < 0) {
pr_err("failed to register helpers\n");
return ret;
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index b2fe6554b9cf..d58da135b5fd 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -577,14 +577,8 @@ static const struct nf_conntrack_expect_policy h245_exp_policy = {
.timeout = 240,
};
-static struct nf_conntrack_helper nf_conntrack_helper_h245 __read_mostly = {
- .name = "H.245",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_UNSPEC,
- .tuple.dst.protonum = IPPROTO_UDP,
- .help = h245_help,
- .expect_policy = &h245_exp_policy,
-};
+static struct nf_conntrack_helper nf_conntrack_helper_h245 __read_mostly;
+static struct nf_conntrack_helper *nf_conntrack_helper_h245_ptr __read_mostly;
int get_h225_addr(struct nf_conn *ct, unsigned char *data,
TransportAddress *taddr,
@@ -1140,26 +1134,8 @@ static const struct nf_conntrack_expect_policy q931_exp_policy = {
.timeout = 240,
};
-static struct nf_conntrack_helper nf_conntrack_helper_q931[] __read_mostly = {
- {
- .name = "Q.931",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_INET,
- .tuple.src.u.tcp.port = cpu_to_be16(Q931_PORT),
- .tuple.dst.protonum = IPPROTO_TCP,
- .help = q931_help,
- .expect_policy = &q931_exp_policy,
- },
- {
- .name = "Q.931",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_INET6,
- .tuple.src.u.tcp.port = cpu_to_be16(Q931_PORT),
- .tuple.dst.protonum = IPPROTO_TCP,
- .help = q931_help,
- .expect_policy = &q931_exp_policy,
- },
-};
+static struct nf_conntrack_helper nf_conntrack_helper_q931[2] __read_mostly;
+static struct nf_conntrack_helper *nf_conntrack_helper_q931_ptr[2] __read_mostly;
static unsigned char *get_udp_data(struct sk_buff *skb, unsigned int protoff,
int *datalen)
@@ -1711,59 +1687,60 @@ static const struct nf_conntrack_expect_policy ras_exp_policy = {
.timeout = 240,
};
-static struct nf_conntrack_helper nf_conntrack_helper_ras[] __read_mostly = {
- {
- .name = "RAS",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_INET,
- .tuple.src.u.udp.port = cpu_to_be16(RAS_PORT),
- .tuple.dst.protonum = IPPROTO_UDP,
- .help = ras_help,
- .expect_policy = &ras_exp_policy,
- },
- {
- .name = "RAS",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_INET6,
- .tuple.src.u.udp.port = cpu_to_be16(RAS_PORT),
- .tuple.dst.protonum = IPPROTO_UDP,
- .help = ras_help,
- .expect_policy = &ras_exp_policy,
- },
-};
+static struct nf_conntrack_helper nf_conntrack_helper_ras[2] __read_mostly;
+static struct nf_conntrack_helper *nf_conntrack_helper_ras_ptr[2] __read_mostly;
static int __init h323_helper_init(void)
{
int ret;
- ret = nf_conntrack_helper_register(&nf_conntrack_helper_h245);
+ nf_ct_helper_init(&nf_conntrack_helper_ras[0], AF_INET, IPPROTO_UDP,
+ "RAS", RAS_PORT, RAS_PORT, RAS_PORT,
+ &ras_exp_policy, 0, ras_help, NULL, THIS_MODULE);
+ nf_ct_helper_init(&nf_conntrack_helper_ras[1], AF_INET6, IPPROTO_UDP,
+ "RAS", RAS_PORT, RAS_PORT, RAS_PORT,
+ &ras_exp_policy, 0, ras_help, NULL, THIS_MODULE);
+ nf_ct_helper_init(&nf_conntrack_helper_h245, AF_UNSPEC, IPPROTO_UDP,
+ "H.245", 0, 0, 0,
+ &h245_exp_policy, 0, h245_help, NULL, THIS_MODULE);
+ nf_ct_helper_init(&nf_conntrack_helper_q931[0], AF_INET, IPPROTO_TCP,
+ "Q.931", Q931_PORT, Q931_PORT, Q931_PORT,
+ &q931_exp_policy, 0, q931_help, NULL, THIS_MODULE);
+ nf_ct_helper_init(&nf_conntrack_helper_q931[1], AF_INET6, IPPROTO_TCP,
+ "Q.931", Q931_PORT, Q931_PORT, Q931_PORT,
+ &q931_exp_policy, 0, q931_help, NULL, THIS_MODULE);
+
+ ret = nf_conntrack_helper_register(&nf_conntrack_helper_h245,
+ &nf_conntrack_helper_h245_ptr);
if (ret < 0)
return ret;
ret = nf_conntrack_helpers_register(nf_conntrack_helper_q931,
- ARRAY_SIZE(nf_conntrack_helper_q931));
+ ARRAY_SIZE(nf_conntrack_helper_q931),
+ nf_conntrack_helper_q931_ptr);
if (ret < 0)
goto err1;
ret = nf_conntrack_helpers_register(nf_conntrack_helper_ras,
- ARRAY_SIZE(nf_conntrack_helper_ras));
+ ARRAY_SIZE(nf_conntrack_helper_ras),
+ nf_conntrack_helper_ras_ptr);
if (ret < 0)
goto err2;
return 0;
err2:
- nf_conntrack_helpers_unregister(nf_conntrack_helper_q931,
- ARRAY_SIZE(nf_conntrack_helper_q931));
+ nf_conntrack_helpers_unregister(nf_conntrack_helper_q931_ptr,
+ ARRAY_SIZE(nf_conntrack_helper_q931_ptr));
err1:
- nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
+ nf_conntrack_helper_unregister(nf_conntrack_helper_h245_ptr);
return ret;
}
static void __exit h323_helper_exit(void)
{
- nf_conntrack_helpers_unregister(nf_conntrack_helper_ras,
+ nf_conntrack_helpers_unregister(nf_conntrack_helper_ras_ptr,
ARRAY_SIZE(nf_conntrack_helper_ras));
- nf_conntrack_helpers_unregister(nf_conntrack_helper_q931,
+ nf_conntrack_helpers_unregister(nf_conntrack_helper_q931_ptr,
ARRAY_SIZE(nf_conntrack_helper_q931));
- nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
+ nf_conntrack_helper_unregister(nf_conntrack_helper_h245_ptr);
}
static void __exit nf_conntrack_h323_fini(void)
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 17e971bd4c74..b5b76e3a6ba0 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -347,7 +347,7 @@ void nf_ct_helper_log(struct sk_buff *skb, const struct nf_conn *ct,
}
EXPORT_SYMBOL_GPL(nf_ct_helper_log);
-int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
+int __nf_conntrack_helper_register(struct nf_conntrack_helper *me)
{
struct nf_conntrack_tuple_mask mask = { .src.u.all = htons(0xFFFF) };
unsigned int h = helper_hash(&me->tuple);
@@ -394,6 +394,33 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
mutex_unlock(&nf_ct_helper_mutex);
return ret;
}
+EXPORT_SYMBOL_GPL(__nf_conntrack_helper_register);
+
+int nf_conntrack_helper_register(struct nf_conntrack_helper *me,
+ struct nf_conntrack_helper **helper_ptr)
+{
+ struct nf_conntrack_helper *new_helper;
+ int err;
+
+ new_helper = kzalloc_obj(*new_helper, GFP_KERNEL_ACCOUNT);
+ if (!new_helper)
+ return -ENOMEM;
+
+ memcpy(new_helper, me, sizeof(*new_helper));
+
+ err = __nf_conntrack_helper_register(new_helper);
+ if (err < 0)
+ goto err_helper;
+
+ *helper_ptr = new_helper;
+
+ return 0;
+
+err_helper:
+ kfree(new_helper);
+
+ return err;
+}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_register);
static bool expect_iter_me(struct nf_conntrack_expect *exp, void *data)
@@ -430,6 +457,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
* last step, this ensures rcu readers of exp->helper are done.
* No need for another synchronize_rcu() here.
*/
+ kfree(me);
}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
@@ -445,11 +473,12 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
struct nf_conn *ct),
struct module *module)
{
+ memset(helper, 0, sizeof(*helper));
+
helper->tuple.src.l3num = l3num;
helper->tuple.dst.protonum = protonum;
helper->tuple.src.u.all = htons(spec_port);
- helper->expect_policy = exp_pol;
- helper->expect_class_max = expect_class_max;
+
helper->help = help;
helper->from_nlattr = from_nlattr;
helper->me = module;
@@ -460,34 +489,54 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
snprintf(helper->name, sizeof(helper->name), "%s", name);
else
snprintf(helper->name, sizeof(helper->name), "%s-%u", name, id);
+
+ if (WARN_ON_ONCE(expect_class_max >= NF_CT_MAX_EXPECT_CLASSES))
+ return;
+
+ memcpy(helper->expect_policy, exp_pol,
+ (expect_class_max + 1) * sizeof(*exp_pol));
+ helper->expect_class_max = expect_class_max;
}
EXPORT_SYMBOL_GPL(nf_ct_helper_init);
int nf_conntrack_helpers_register(struct nf_conntrack_helper *helper,
- unsigned int n)
+ unsigned int n, struct nf_conntrack_helper **helper_ptr)
{
+ struct nf_conntrack_helper *new_helper;
unsigned int i;
int err = 0;
for (i = 0; i < n; i++) {
- err = nf_conntrack_helper_register(&helper[i]);
- if (err < 0)
+ new_helper = kzalloc_obj(*new_helper, GFP_KERNEL_ACCOUNT);
+ if (!new_helper)
goto err;
+
+ memcpy(new_helper, &helper[i], sizeof(*new_helper));
+
+ err = __nf_conntrack_helper_register(new_helper);
+ if (err < 0)
+ goto err_helper;
+
+ helper_ptr[i] = new_helper;
}
return err;
+err_helper:
+ kfree(new_helper);
err:
if (i > 0)
- nf_conntrack_helpers_unregister(helper, i);
+ nf_conntrack_helpers_unregister(helper_ptr, i);
return err;
}
EXPORT_SYMBOL_GPL(nf_conntrack_helpers_register);
-void nf_conntrack_helpers_unregister(struct nf_conntrack_helper *helper,
- unsigned int n)
+void nf_conntrack_helpers_unregister(struct nf_conntrack_helper **helper,
+ unsigned int n)
{
- while (n-- > 0)
- nf_conntrack_helper_unregister(&helper[n]);
+ while (n-- > 0) {
+ nf_conntrack_helper_unregister(helper[n]);
+ helper[n] = NULL;
+ }
}
EXPORT_SYMBOL_GPL(nf_conntrack_helpers_unregister);
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 522183b9a604..db1c7718dcbe 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -255,6 +255,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
}
static struct nf_conntrack_helper irc[MAX_PORTS] __read_mostly;
+static struct nf_conntrack_helper *irc_ptr[MAX_PORTS] __read_mostly;
static struct nf_conntrack_expect_policy irc_exp_policy;
static int __init nf_conntrack_irc_init(void)
@@ -289,7 +290,7 @@ static int __init nf_conntrack_irc_init(void)
0, help, NULL, THIS_MODULE);
}
- ret = nf_conntrack_helpers_register(&irc[0], ports_c);
+ ret = nf_conntrack_helpers_register(&irc[0], ports_c, irc_ptr);
if (ret) {
pr_err("failed to register helpers\n");
kfree(irc_buffer);
@@ -301,7 +302,7 @@ static int __init nf_conntrack_irc_init(void)
static void __exit nf_conntrack_irc_fini(void)
{
- nf_conntrack_helpers_unregister(irc, ports_c);
+ nf_conntrack_helpers_unregister(irc_ptr, ports_c);
kfree(irc_buffer);
}
diff --git a/net/netfilter/nf_conntrack_netbios_ns.c b/net/netfilter/nf_conntrack_netbios_ns.c
index 55415f011943..852454926ef9 100644
--- a/net/netfilter/nf_conntrack_netbios_ns.c
+++ b/net/netfilter/nf_conntrack_netbios_ns.c
@@ -44,22 +44,20 @@ static int netbios_ns_help(struct sk_buff *skb, unsigned int protoff,
return nf_conntrack_broadcast_help(skb, ct, ctinfo, timeout);
}
-static struct nf_conntrack_helper helper __read_mostly = {
- .name = HELPER_NAME,
- .tuple.src.l3num = NFPROTO_IPV4,
- .tuple.src.u.udp.port = cpu_to_be16(NMBD_PORT),
- .tuple.dst.protonum = IPPROTO_UDP,
- .me = THIS_MODULE,
- .help = netbios_ns_help,
- .expect_policy = &exp_policy,
-};
+static struct nf_conntrack_helper helper __read_mostly;
+static struct nf_conntrack_helper *helper_ptr __read_mostly;
static int __init nf_conntrack_netbios_ns_init(void)
{
NF_CT_HELPER_BUILD_BUG_ON(0);
exp_policy.timeout = timeout;
- return nf_conntrack_helper_register(&helper);
+
+ nf_ct_helper_init(&helper, AF_INET, IPPROTO_UDP, HELPER_NAME,
+ NMBD_PORT, NMBD_PORT, NMBD_PORT,
+ &exp_policy, 0, netbios_ns_help, NULL, THIS_MODULE);
+
+ return nf_conntrack_helper_register(&helper, &helper_ptr);
}
static void __exit nf_conntrack_netbios_ns_fini(void)
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 4c679638df06..1c0b310d4b63 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -376,7 +376,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
}
static int
-pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
+pptp_ptrbound_pkt(struct sk_buff *skb, unsigned int protoff,
struct PptpControlHeader *ctlh,
union pptp_ctrl_union *pptpReq,
unsigned int reqlen,
@@ -567,7 +567,7 @@ conntrack_pptp_help(struct sk_buff *skb, unsigned int protoff,
* established from PNS->PAC. However, RFC makes no guarantee */
if (dir == IP_CT_DIR_ORIGINAL)
/* client -> server (PNS -> PAC) */
- ret = pptp_outbound_pkt(skb, protoff, ctlh, pptpReq, reqlen, ct,
+ ret = pptp_ptrbound_pkt(skb, protoff, ctlh, pptpReq, reqlen, ct,
ctinfo);
else
/* server -> client (PAC -> PNS) */
@@ -586,27 +586,25 @@ static const struct nf_conntrack_expect_policy pptp_exp_policy = {
};
/* control protocol helper */
-static struct nf_conntrack_helper pptp __read_mostly = {
- .name = "pptp",
- .me = THIS_MODULE,
- .tuple.src.l3num = AF_INET,
- .tuple.src.u.tcp.port = cpu_to_be16(PPTP_CONTROL_PORT),
- .tuple.dst.protonum = IPPROTO_TCP,
- .help = conntrack_pptp_help,
- .destroy = pptp_destroy_siblings,
- .expect_policy = &pptp_exp_policy,
-};
+static struct nf_conntrack_helper pptp __read_mostly;
+static struct nf_conntrack_helper *pptp_ptr __read_mostly;
static int __init nf_conntrack_pptp_init(void)
{
NF_CT_HELPER_BUILD_BUG_ON(sizeof(struct nf_ct_pptp_master));
- return nf_conntrack_helper_register(&pptp);
+ nf_ct_helper_init(&pptp, AF_INET, IPPROTO_TCP,
+ "pptp", PPTP_CONTROL_PORT, PPTP_CONTROL_PORT, PPTP_CONTROL_PORT,
+ &pptp_exp_policy, 0, conntrack_pptp_help, NULL, THIS_MODULE);
+
+ pptp.destroy = pptp_destroy_siblings;
+
+ return nf_conntrack_helper_register(&pptp, &pptp_ptr);
}
static void __exit nf_conntrack_pptp_fini(void)
{
- nf_conntrack_helper_unregister(&pptp);
+ nf_conntrack_helper_unregister(pptp_ptr);
}
module_init(nf_conntrack_pptp_init);
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 13dc421fc4f5..a7f7b07ba0c2 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -167,6 +167,7 @@ static int help(struct sk_buff *skb,
}
static struct nf_conntrack_helper sane[MAX_PORTS * 2] __read_mostly;
+static struct nf_conntrack_helper *sane_ptr[MAX_PORTS * 2] __read_mostly;
static const struct nf_conntrack_expect_policy sane_exp_policy = {
.max_expected = 1,
@@ -175,7 +176,7 @@ static const struct nf_conntrack_expect_policy sane_exp_policy = {
static void __exit nf_conntrack_sane_fini(void)
{
- nf_conntrack_helpers_unregister(sane, ports_c * 2);
+ nf_conntrack_helpers_unregister(sane_ptr, ports_c * 2);
}
static int __init nf_conntrack_sane_init(void)
@@ -200,7 +201,7 @@ static int __init nf_conntrack_sane_init(void)
THIS_MODULE);
}
- ret = nf_conntrack_helpers_register(sane, ports_c * 2);
+ ret = nf_conntrack_helpers_register(sane, ports_c * 2, sane_ptr);
if (ret < 0) {
pr_err("failed to register helpers\n");
return ret;
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index e69941f1a101..2c78a3e1dab5 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1731,6 +1731,7 @@ static int sip_help_udp(struct sk_buff *skb, unsigned int protoff,
}
static struct nf_conntrack_helper sip[MAX_PORTS * 4] __read_mostly;
+static struct nf_conntrack_helper *sip_ptr[MAX_PORTS * 4] __read_mostly;
static const struct nf_conntrack_expect_policy sip_exp_policy[SIP_EXPECT_MAX + 1] = {
[SIP_EXPECT_SIGNALLING] = {
@@ -1757,7 +1758,7 @@ static const struct nf_conntrack_expect_policy sip_exp_policy[SIP_EXPECT_MAX + 1
static void __exit nf_conntrack_sip_fini(void)
{
- nf_conntrack_helpers_unregister(sip, ports_c * 4);
+ nf_conntrack_helpers_unregister(sip_ptr, ports_c * 4);
}
static int __init nf_conntrack_sip_init(void)
@@ -1788,7 +1789,7 @@ static int __init nf_conntrack_sip_init(void)
NULL, THIS_MODULE);
}
- ret = nf_conntrack_helpers_register(sip, ports_c * 4);
+ ret = nf_conntrack_helpers_register(sip, ports_c * 4, sip_ptr);
if (ret < 0) {
pr_err("failed to register helpers\n");
return ret;
diff --git a/net/netfilter/nf_conntrack_snmp.c b/net/netfilter/nf_conntrack_snmp.c
index 7b7eed43c54f..b6fce5703fce 100644
--- a/net/netfilter/nf_conntrack_snmp.c
+++ b/net/netfilter/nf_conntrack_snmp.c
@@ -47,25 +47,24 @@ static struct nf_conntrack_expect_policy exp_policy = {
.max_expected = 1,
};
-static struct nf_conntrack_helper helper __read_mostly = {
- .name = "snmp",
- .tuple.src.l3num = NFPROTO_IPV4,
- .tuple.src.u.udp.port = cpu_to_be16(SNMP_PORT),
- .tuple.dst.protonum = IPPROTO_UDP,
- .me = THIS_MODULE,
- .help = snmp_conntrack_help,
- .expect_policy = &exp_policy,
-};
+static struct nf_conntrack_helper helper __read_mostly;
+static struct nf_conntrack_helper *helper_ptr __read_mostly;
static int __init nf_conntrack_snmp_init(void)
{
exp_policy.timeout = timeout;
- return nf_conntrack_helper_register(&helper);
+
+ nf_ct_helper_init(&helper, AF_INET, IPPROTO_UDP,
+ "snmp", SNMP_PORT, SNMP_PORT, SNMP_PORT,
+ &exp_policy, 0, snmp_conntrack_help, NULL,
+ THIS_MODULE);
+
+ return nf_conntrack_helper_register(&helper, &helper_ptr);
}
static void __exit nf_conntrack_snmp_fini(void)
{
- nf_conntrack_helper_unregister(&helper);
+ nf_conntrack_helper_unregister(helper_ptr);
}
module_init(nf_conntrack_snmp_init);
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index a2e6833a0bf7..4393c435aa35 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -96,6 +96,7 @@ static int tftp_help(struct sk_buff *skb,
}
static struct nf_conntrack_helper tftp[MAX_PORTS * 2] __read_mostly;
+static struct nf_conntrack_helper *tftp_ptr[MAX_PORTS * 2] __read_mostly;
static const struct nf_conntrack_expect_policy tftp_exp_policy = {
.max_expected = 1,
@@ -104,7 +105,7 @@ static const struct nf_conntrack_expect_policy tftp_exp_policy = {
static void __exit nf_conntrack_tftp_fini(void)
{
- nf_conntrack_helpers_unregister(tftp, ports_c * 2);
+ nf_conntrack_helpers_unregister(tftp_ptr, ports_c * 2);
}
static int __init nf_conntrack_tftp_init(void)
@@ -127,7 +128,7 @@ static int __init nf_conntrack_tftp_init(void)
THIS_MODULE);
}
- ret = nf_conntrack_helpers_register(tftp, ports_c * 2);
+ ret = nf_conntrack_helpers_register(tftp, ports_c * 2, tftp_ptr);
if (ret < 0) {
pr_err("failed to register helpers\n");
return ret;
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 61a2407b53bd..67da64e271de 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -176,7 +176,6 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
const struct nlattr *attr)
{
int i, ret;
- struct nf_conntrack_expect_policy *expect_policy;
struct nlattr *tb[NFCTH_POLICY_SET_MAX+1];
unsigned int class_max;
@@ -195,26 +194,19 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
if (class_max > NF_CT_MAX_EXPECT_CLASSES)
return -EOVERFLOW;
- expect_policy = kzalloc_objs(struct nf_conntrack_expect_policy,
- class_max);
- if (expect_policy == NULL)
- return -ENOMEM;
-
for (i = 0; i < class_max; i++) {
if (!tb[NFCTH_POLICY_SET+i])
goto err;
- ret = nfnl_cthelper_expect_policy(&expect_policy[i],
+ ret = nfnl_cthelper_expect_policy(&helper->expect_policy[i],
tb[NFCTH_POLICY_SET+i]);
if (ret < 0)
goto err;
}
helper->expect_class_max = class_max - 1;
- helper->expect_policy = expect_policy;
return 0;
err:
- kfree(expect_policy);
return -EINVAL;
}
@@ -244,7 +236,7 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
size = ntohl(nla_get_be32(tb[NFCTH_PRIV_DATA_LEN]));
if (size > sizeof_field(struct nf_conn_help, data)) {
ret = -ENOMEM;
- goto err2;
+ goto err1;
}
helper->data_len = size;
@@ -273,14 +265,12 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
}
}
- ret = nf_conntrack_helper_register(helper);
+ ret = __nf_conntrack_helper_register(helper);
if (ret < 0)
- goto err2;
+ goto err1;
list_add_tail(&nfcth->list, &nfnl_cthelper_list);
return 0;
-err2:
- kfree(helper->expect_policy);
err1:
kfree(nfcth);
return ret;
@@ -723,7 +713,6 @@ static int nfnl_cthelper_del(struct sk_buff *skb, const struct nfnl_info *info,
if (refcount_dec_if_one(&cur->refcnt)) {
found = true;
nf_conntrack_helper_unregister(cur);
- kfree(cur->expect_policy);
list_del(&nlcth->list);
kfree(nlcth);
@@ -799,7 +788,6 @@ static void __exit nfnl_cthelper_exit(void)
cur = &nlcth->helper;
nf_conntrack_helper_unregister(cur);
- kfree(cur->expect_policy);
kfree(nlcth);
}
}
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH nf-next 4/6] netfilter: nf_conntrack_pptp: move GRE specific cleanup to GRE tracker
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
` (2 preceding siblings ...)
2026-05-26 16:40 ` [PATCH nf-next 3/6] netfilter: nf_conntrack_helper: dynamically allocate struct nf_conntrack_helper Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath Pablo Neira Ayuso
2026-05-26 16:40 ` [PATCH nf-next 6/6] netfilter: conntrack: revert ct extension genid infrastructure Pablo Neira Ayuso
5 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
Move the GRE specific cleanup to nf_conntrack_proto_gre.c to ensure that
the .destroy callback for the pptp helper is still reachable by existing
conntrack entries while pptp module is being removed.
This is a preparation patch, no functional changes are intended.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
.../net/netfilter/ipv4/nf_conntrack_ipv4.h | 4 ++
net/netfilter/nf_conntrack_pptp.c | 63 +------------------
net/netfilter/nf_conntrack_proto_gre.c | 61 ++++++++++++++++++
3 files changed, 67 insertions(+), 61 deletions(-)
diff --git a/include/net/netfilter/ipv4/nf_conntrack_ipv4.h b/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
index b39417ad955e..0b07d5e69c15 100644
--- a/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
+++ b/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
@@ -20,4 +20,8 @@ extern const struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp;
extern const struct nf_conntrack_l4proto nf_conntrack_l4proto_gre;
#endif
+#if IS_ENABLED(CONFIG_NF_CONNTRACK_PPTP)
+void gre_pptp_destroy_siblings(struct nf_conn *ct);
+#endif
+
#endif /*_NF_CONNTRACK_IPV4_H*/
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 1c0b310d4b63..16147cf71088 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -124,65 +124,6 @@ static void pptp_expectfn(struct nf_conn *ct,
}
}
-static int destroy_sibling_or_exp(struct net *net, struct nf_conn *ct,
- const struct nf_conntrack_tuple *t)
-{
- const struct nf_conntrack_tuple_hash *h;
- const struct nf_conntrack_zone *zone;
- struct nf_conntrack_expect *exp;
- struct nf_conn *sibling;
-
- pr_debug("trying to timeout ct or exp for tuple ");
- nf_ct_dump_tuple(t);
-
- zone = nf_ct_zone(ct);
- h = nf_conntrack_find_get(net, zone, t);
- if (h) {
- sibling = nf_ct_tuplehash_to_ctrack(h);
- pr_debug("setting timeout of conntrack %p to 0\n", sibling);
- sibling->proto.gre.timeout = 0;
- sibling->proto.gre.stream_timeout = 0;
- nf_ct_kill(sibling);
- nf_ct_put(sibling);
- return 1;
- } else {
- exp = nf_ct_expect_find_get(net, zone, t);
- if (exp) {
- pr_debug("unexpect_related of expect %p\n", exp);
- nf_ct_unexpect_related(exp);
- nf_ct_expect_put(exp);
- return 1;
- }
- }
- return 0;
-}
-
-/* timeout GRE data connections */
-static void pptp_destroy_siblings(struct nf_conn *ct)
-{
- struct net *net = nf_ct_net(ct);
- const struct nf_ct_pptp_master *ct_pptp_info = nfct_help_data(ct);
- struct nf_conntrack_tuple t;
-
- nf_ct_gre_keymap_destroy(ct);
-
- /* try original (pns->pac) tuple */
- memcpy(&t, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple, sizeof(t));
- t.dst.protonum = IPPROTO_GRE;
- t.src.u.gre.key = ct_pptp_info->pns_call_id;
- t.dst.u.gre.key = ct_pptp_info->pac_call_id;
- if (!destroy_sibling_or_exp(net, ct, &t))
- pr_debug("failed to timeout original pns->pac ct/exp\n");
-
- /* try reply (pac->pns) tuple */
- memcpy(&t, &ct->tuplehash[IP_CT_DIR_REPLY].tuple, sizeof(t));
- t.dst.protonum = IPPROTO_GRE;
- t.src.u.gre.key = ct_pptp_info->pac_call_id;
- t.dst.u.gre.key = ct_pptp_info->pns_call_id;
- if (!destroy_sibling_or_exp(net, ct, &t))
- pr_debug("failed to timeout reply pac->pns ct/exp\n");
-}
-
/* expect GRE connections (PNS->PAC and PAC->PNS direction) */
static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
{
@@ -347,7 +288,7 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
info->cstate = PPTP_CALL_NONE;
/* untrack this call id, unexpect GRE packets */
- pptp_destroy_siblings(ct);
+ gre_pptp_destroy_siblings(ct);
break;
case PPTP_WAN_ERROR_NOTIFY:
@@ -597,7 +538,7 @@ static int __init nf_conntrack_pptp_init(void)
"pptp", PPTP_CONTROL_PORT, PPTP_CONTROL_PORT, PPTP_CONTROL_PORT,
&pptp_exp_policy, 0, conntrack_pptp_help, NULL, THIS_MODULE);
- pptp.destroy = pptp_destroy_siblings;
+ pptp.destroy = gre_pptp_destroy_siblings;
return nf_conntrack_helper_register(&pptp, &pptp_ptr);
}
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 94c19bc4edc5..a3f907416ff3 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -293,6 +293,67 @@ gre_timeout_nla_policy[CTA_TIMEOUT_GRE_MAX+1] = {
};
#endif /* CONFIG_NF_CONNTRACK_TIMEOUT */
+#if IS_ENABLED(CONFIG_NF_CONNTRACK_PPTP)
+static int destroy_sibling_or_exp(struct net *net, struct nf_conn *ct,
+ const struct nf_conntrack_tuple *t)
+{
+ const struct nf_conntrack_tuple_hash *h;
+ const struct nf_conntrack_zone *zone;
+ struct nf_conntrack_expect *exp;
+ struct nf_conn *sibling;
+
+ pr_debug("trying to timeout ct or exp for tuple ");
+ nf_ct_dump_tuple(t);
+
+ zone = nf_ct_zone(ct);
+ h = nf_conntrack_find_get(net, zone, t);
+ if (h) {
+ sibling = nf_ct_tuplehash_to_ctrack(h);
+ pr_debug("setting timeout of conntrack %p to 0\n", sibling);
+ sibling->proto.gre.timeout = 0;
+ sibling->proto.gre.stream_timeout = 0;
+ nf_ct_kill(sibling);
+ nf_ct_put(sibling);
+ return 1;
+ } else {
+ exp = nf_ct_expect_find_get(net, zone, t);
+ if (exp) {
+ pr_debug("unexpect_related of expect %p\n", exp);
+ nf_ct_unexpect_related(exp);
+ nf_ct_expect_put(exp);
+ return 1;
+ }
+ }
+ return 0;
+}
+
+void gre_pptp_destroy_siblings(struct nf_conn *ct)
+{
+ struct net *net = nf_ct_net(ct);
+ const struct nf_ct_pptp_master *ct_pptp_info = nfct_help_data(ct);
+ struct nf_conntrack_tuple t;
+
+ nf_ct_gre_keymap_destroy(ct);
+
+ /* try original (pns->pac) tuple */
+ memcpy(&t, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple, sizeof(t));
+ t.dst.protonum = IPPROTO_GRE;
+ t.src.u.gre.key = ct_pptp_info->pns_call_id;
+ t.dst.u.gre.key = ct_pptp_info->pac_call_id;
+ if (!destroy_sibling_or_exp(net, ct, &t))
+ pr_debug("failed to timeout original pns->pac ct/exp\n");
+
+ /* try reply (pac->pns) tuple */
+ memcpy(&t, &ct->tuplehash[IP_CT_DIR_REPLY].tuple, sizeof(t));
+ t.dst.protonum = IPPROTO_GRE;
+ t.src.u.gre.key = ct_pptp_info->pac_call_id;
+ t.dst.u.gre.key = ct_pptp_info->pns_call_id;
+ if (!destroy_sibling_or_exp(net, ct, &t))
+ pr_debug("failed to timeout reply pac->pns ct/exp\n");
+}
+EXPORT_SYMBOL_GPL(gre_pptp_destroy_siblings);
+#endif
+
void nf_conntrack_gre_init_net(struct net *net)
{
struct nf_gre_net *net_gre = gre_pernet(net);
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
` (3 preceding siblings ...)
2026-05-26 16:40 ` [PATCH nf-next 4/6] netfilter: nf_conntrack_pptp: move GRE specific cleanup to GRE tracker Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
2026-05-26 17:54 ` Florian Westphal
2026-05-26 16:40 ` [PATCH nf-next 6/6] netfilter: conntrack: revert ct extension genid infrastructure Pablo Neira Ayuso
5 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
There is already a refcount for control plane, to ensure the helper
does not go away if it is used by rulesets.
This patch adds a new ->ct_refcnt field to struct nf_conntrack_helper
which is bumped when the helper is used by the ct helper extension. Drop
this reference count when the conntrack entry is released. This is a
packet path refcount which ensures that struct nf_conntrack_helper
remains in place for tricky scenarios where a packet sits in nfqueue, or
elsewhere, with a conntrack that refers to this helper.
On helper removal, the help callback is set to NULL to disable it from
packet path and, after rcu grace period, existing expectations are
removed. Update ctnetlink to disable access to .to_nlattr and
.from_nlattr if the helper is going away.
Remove nf_queue_nf_hook_drop() since it has proven not to be effective
because packets with unconfirmed conntracks which are still flying to
sit in nfqueue.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack_helper.h | 25 +++++++++++++++---
net/netfilter/nf_conntrack_core.c | 3 ++-
net/netfilter/nf_conntrack_helper.c | 28 ++++++---------------
net/netfilter/nf_conntrack_netlink.c | 12 ++++++---
net/netfilter/nf_conntrack_ovs.c | 14 ++++++++++-
net/netfilter/nf_conntrack_proto.c | 15 +++++++----
net/netfilter/nft_ct.c | 2 +-
net/netfilter/xt_CT.c | 7 +++---
8 files changed, 66 insertions(+), 40 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 1956bc12bf56..a03cb4e59ea9 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -35,20 +35,23 @@ enum nf_ct_helper_flags {
struct nf_conntrack_helper {
struct hlist_node hnode; /* Internal use. */
+ struct rcu_head rcu;
+
char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
refcount_t refcnt;
struct module *me; /* pointer to self */
struct nf_conntrack_expect_policy expect_policy[NF_CT_MAX_EXPECT_CLASSES];
+ refcount_t ct_refcnt;
+
/* Tuple of things we will help (compared against server response) */
struct nf_conntrack_tuple tuple;
/* Function to call when data passes; return verdict, or -1 to
invalidate. */
- int (*help)(struct sk_buff *skb,
- unsigned int protoff,
- struct nf_conn *ct,
- enum ip_conntrack_info conntrackinfo);
+ int __rcu (*help)(struct sk_buff *skb, unsigned int protoff,
+ struct nf_conn *ct,
+ enum ip_conntrack_info conntrackinfo);
void (*destroy)(struct nf_conn *ct);
@@ -138,6 +141,20 @@ static inline void *nfct_help_data(const struct nf_conn *ct)
return (void *)help->data;
}
+static inline void nf_ct_help_put(const struct nf_conn *ct)
+{
+ struct nf_conntrack_helper *helper;
+ struct nf_conn_help *help;
+
+ help = nfct_help(ct);
+ if (!help)
+ return;
+
+ helper = rcu_dereference(help->helper);
+ if (helper && refcount_dec_and_test(&helper->ct_refcnt))
+ kfree_rcu(helper, rcu);
+}
+
int nf_conntrack_helper_init(void);
void nf_conntrack_helper_fini(void);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 63159c070c3a..493748f792de 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1739,6 +1739,7 @@ void nf_conntrack_free(struct nf_conn *ct)
nat_hook->remove_nat_bysrc(ct);
}
+ nf_ct_help_put(ct);
nf_ct_timeout_put(ct);
rcu_read_unlock();
@@ -1822,7 +1823,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
assign_helper = rcu_dereference(exp->assign_helper);
if (assign_helper) {
help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
- if (help)
+ if (help && refcount_inc_not_zero(&assign_helper->ct_refcnt))
rcu_assign_pointer(help->helper, assign_helper);
}
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index b5b76e3a6ba0..0fe15d749d91 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -237,20 +237,6 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
}
EXPORT_SYMBOL_GPL(__nf_ct_try_assign_helper);
-/* appropriate ct lock protecting must be taken by caller */
-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);
- }
-
- /* We are not intended to delete this conntrack. */
- return 0;
-}
-
void nf_ct_helper_destroy(struct nf_conn *ct)
{
struct nf_conn_help *help = nfct_help(ct);
@@ -388,6 +374,7 @@ int __nf_conntrack_helper_register(struct nf_conntrack_helper *me)
}
}
refcount_set(&me->refcnt, 1);
+ refcount_set(&me->ct_refcnt, 1);
hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
nf_ct_helper_count++;
out:
@@ -445,19 +432,18 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
nf_ct_helper_count--;
mutex_unlock(&nf_ct_helper_mutex);
+ /* This helper is going away, disable it. */
+ 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_ct_expect_iterate_destroy(expect_iter_me, me);
- nf_ct_iterate_destroy(unhelp, me);
- /* nf_ct_iterate_destroy() does an unconditional synchronize_rcu() as
- * last step, this ensures rcu readers of exp->helper are done.
- * No need for another synchronize_rcu() here.
- */
- kfree(me);
+ if (refcount_dec_and_test(&me->ct_refcnt))
+ kfree_rcu(me, rcu);
}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
@@ -479,7 +465,7 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
helper->tuple.dst.protonum = protonum;
helper->tuple.src.u.all = htons(spec_port);
- helper->help = help;
+ rcu_assign_pointer(helper->help, help);
helper->from_nlattr = from_nlattr;
helper->me = module;
snprintf(helper->nat_mod_name, sizeof(helper->nat_mod_name),
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index befa7e83ee49..4ba6ded8a29f 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -240,7 +240,8 @@ static int ctnetlink_dump_helpinfo(struct sk_buff *skb,
if (nla_put_string(skb, CTA_HELP_NAME, helper->name))
goto nla_put_failure;
- if (helper->to_nlattr)
+ if (rcu_access_pointer(helper->help) &&
+ helper->to_nlattr)
helper->to_nlattr(skb, ct);
nla_nest_end(skb, nest_helper);
@@ -1974,7 +1975,8 @@ static int ctnetlink_change_helper(struct nf_conn *ct,
if (help) {
if (rcu_access_pointer(help->helper) == helper) {
/* update private helper data if allowed. */
- if (helper->from_nlattr)
+ if (rcu_access_pointer(helper->help) &&
+ helper->from_nlattr)
helper->from_nlattr(helpinfo, ct);
err = 0;
} else
@@ -2289,12 +2291,14 @@ ctnetlink_create_conntrack(struct net *net,
goto err2;
}
/* set private helper data if allowed. */
- if (helper->from_nlattr)
+ if (rcu_access_pointer(helper->help) &&
+ helper->from_nlattr)
helper->from_nlattr(helpinfo, ct);
/* disable helper auto-assignment for this entry */
ct->status |= IPS_HELPER;
- RCU_INIT_POINTER(help->helper, helper);
+ if (refcount_inc_not_zero(&helper->ct_refcnt))
+ RCU_INIT_POINTER(help->helper, helper);
}
}
diff --git a/net/netfilter/nf_conntrack_ovs.c b/net/netfilter/nf_conntrack_ovs.c
index a6988eeb1579..ddb2ac6fd982 100644
--- a/net/netfilter/nf_conntrack_ovs.c
+++ b/net/netfilter/nf_conntrack_ovs.c
@@ -12,6 +12,9 @@
int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
enum ip_conntrack_info ctinfo, u16 proto)
{
+ int (*helper_cb)(struct sk_buff *skb, unsigned int protoff,
+ struct nf_conn *ct,
+ enum ip_conntrack_info conntrackinfo);
const struct nf_conntrack_helper *helper;
const struct nf_conn_help *help;
unsigned int protoff;
@@ -60,7 +63,11 @@ int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
if (helper->tuple.dst.protonum != proto)
return NF_ACCEPT;
- err = helper->help(skb, protoff, ct, ctinfo);
+ helper_cb = rcu_dereference(helper->help);
+ if (!helper_cb)
+ return NF_ACCEPT;
+
+ err = helper_cb(skb, protoff, ct, ctinfo);
if (err != NF_ACCEPT)
return err;
@@ -100,6 +107,11 @@ int nf_ct_add_helper(struct nf_conn *ct, const char *name, u8 family,
}
}
#endif
+ if (!refcount_inc_not_zero(&helper->ct_refcnt)) {
+ nf_conntrack_helper_put(helper);
+ return -ENOENT;
+ }
+
rcu_assign_pointer(help->helper, helper);
*hp = helper;
return ret;
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 50ddd3d613e1..ad96896516b6 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -129,6 +129,9 @@ unsigned int nf_confirm(void *priv,
struct sk_buff *skb,
const struct nf_hook_state *state)
{
+ int (*helper_cb)(struct sk_buff *skb, unsigned int protoff,
+ struct nf_conn *ct,
+ enum ip_conntrack_info conntrackinfo);
const struct nf_conn_help *help;
enum ip_conntrack_info ctinfo;
unsigned int protoff;
@@ -175,11 +178,13 @@ unsigned int nf_confirm(void *priv,
/* rcu_read_lock()ed by nf_hook */
helper = rcu_dereference(help->helper);
if (helper) {
- ret = helper->help(skb,
- protoff,
- ct, ctinfo);
- if (ret != NF_ACCEPT)
- return ret;
+ helper_cb = rcu_dereference(helper->help);
+ if (helper_cb) {
+ ret = helper_cb(skb, protoff,
+ ct, ctinfo);
+ if (ret != NF_ACCEPT)
+ return ret;
+ }
}
}
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 85e3d68dfb59..61f63d3a445b 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -1148,7 +1148,7 @@ static void nft_ct_helper_obj_eval(struct nft_object *obj,
return;
help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
- if (help) {
+ if (help && refcount_inc_not_zero(&to_assign->ct_refcnt)) {
rcu_assign_pointer(help->helper, to_assign);
set_bit(IPS_HELPER_BIT, &ct->status);
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index b94f004d5f5c..3b9f4e182468 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -97,6 +97,10 @@ xt_ct_set_helper(struct nf_conn *ct, const char *helper_name,
return -ENOMEM;
}
+ if (!refcount_inc_not_zero(&helper->ct_refcnt)) {
+ nf_conntrack_helper_put(helper);
+ return -ENOENT;
+ }
rcu_assign_pointer(help->helper, helper);
return 0;
}
@@ -284,9 +288,6 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param *par,
struct nf_conn_help *help;
if (ct) {
- if (info->helper[0])
- nf_queue_nf_hook_drop(par->net);
-
help = nfct_help(ct);
xt_ct_put_helper(help);
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath
2026-05-26 16:40 ` [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath Pablo Neira Ayuso
@ 2026-05-26 17:54 ` Florian Westphal
2026-05-26 22:19 ` Pablo Neira Ayuso
0 siblings, 1 reply; 10+ messages in thread
From: Florian Westphal @ 2026-05-26 17:54 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> There is already a refcount for control plane, to ensure the helper
> does not go away if it is used by rulesets.
>
> This patch adds a new ->ct_refcnt field to struct nf_conntrack_helper
> which is bumped when the helper is used by the ct helper extension. Drop
> this reference count when the conntrack entry is released. This is a
> packet path refcount which ensures that struct nf_conntrack_helper
> remains in place for tricky scenarios where a packet sits in nfqueue, or
> elsewhere, with a conntrack that refers to this helper.
>
> On helper removal, the help callback is set to NULL to disable it from
> packet path and, after rcu grace period, existing expectations are
> removed. Update ctnetlink to disable access to .to_nlattr and
> .from_nlattr if the helper is going away.
>
> Remove nf_queue_nf_hook_drop() since it has proven not to be effective
> because packets with unconfirmed conntracks which are still flying to
> sit in nfqueue.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> include/net/netfilter/nf_conntrack_helper.h | 25 +++++++++++++++---
> net/netfilter/nf_conntrack_core.c | 3 ++-
> net/netfilter/nf_conntrack_helper.c | 28 ++++++---------------
> net/netfilter/nf_conntrack_netlink.c | 12 ++++++---
> net/netfilter/nf_conntrack_ovs.c | 14 ++++++++++-
> net/netfilter/nf_conntrack_proto.c | 15 +++++++----
> net/netfilter/nft_ct.c | 2 +-
> net/netfilter/xt_CT.c | 7 +++---
> 8 files changed, 66 insertions(+), 40 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> index 1956bc12bf56..a03cb4e59ea9 100644
> --- a/include/net/netfilter/nf_conntrack_helper.h
> +++ b/include/net/netfilter/nf_conntrack_helper.h
> @@ -35,20 +35,23 @@ enum nf_ct_helper_flags {
> struct nf_conntrack_helper {
> struct hlist_node hnode; /* Internal use. */
>
> + struct rcu_head rcu;
> +
> char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> refcount_t refcnt;
> struct module *me; /* pointer to self */
> struct nf_conntrack_expect_policy expect_policy[NF_CT_MAX_EXPECT_CLASSES];
>
> + refcount_t ct_refcnt;
Why do we need two reference counts? I find this very confusing.
Which refcount frees the structure? And can one refcount hit 0 while
other one is still in use?
> /* Function to call when data passes; return verdict, or -1 to
> invalidate. */
> - int (*help)(struct sk_buff *skb,
> - unsigned int protoff,
> - struct nf_conn *ct,
> - enum ip_conntrack_info conntrackinfo);
> + int __rcu (*help)(struct sk_buff *skb, unsigned int protoff,
> + struct nf_conn *ct,
> + enum ip_conntrack_info conntrackinfo);
>
> void (*destroy)(struct nf_conn *ct);
Why is help RCU protected while other callbacks are not?
'destroy' not being rcu protected implies that the helper module must
remain in memory until after kfree_rcu has released the underlying
storage anyway.
If thats true, why do we need rcu head and kfree_rcu in the first place?
module has to remain in memory until after last possible caller has
called me->destroy(), no? If that is correct, then there is no need for
dynamically allocated storage.
> @@ -445,19 +432,18 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> nf_ct_helper_count--;
> mutex_unlock(&nf_ct_helper_mutex);
>
> + /* This helper is going away, disable it. */
> + rcu_assign_pointer(me->help, NULL);
> +
OK, so this signals pending removal (refcnt can still be elevated) to
prevent new packets/expectations from grabbing another reference.
Correct? Is this a 'dying' flag or is there more to it?
I looked at patched 'nf_conntrack_ftp_fini', but I don't see anything
that spins/waits for completion of referencing entries.
How does ->destroy/to_nlattr/from_nlattr etc. work?
I expected to find something that does a busywait until refcount has
hit 0 to avoid any calls to the removed module.
The existing conntracks still hold a pointer to struct
nf_conntrack_helper, and its refcount can be elevated too, while
function pointers (not help, but others) are stale.
I suspect you need to move the function pointers to an 'op' sub-struct,
so that it can be cleared via single rcu_assign_pointer(me->help_ops, NULL) ?
But that still has one problem: if helper module is gone, how can you
call the destructor?
Maybe we need to accelerate pptp removal so the only user of destroy
is removed?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath
2026-05-26 17:54 ` Florian Westphal
@ 2026-05-26 22:19 ` Pablo Neira Ayuso
2026-05-27 13:39 ` Florian Westphal
0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 22:19 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
Hi Florian,
On Tue, May 26, 2026 at 07:54:51PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> > index 1956bc12bf56..a03cb4e59ea9 100644
> > --- a/include/net/netfilter/nf_conntrack_helper.h
> > +++ b/include/net/netfilter/nf_conntrack_helper.h
> > @@ -35,20 +35,23 @@ enum nf_ct_helper_flags {
> > struct nf_conntrack_helper {
> > struct hlist_node hnode; /* Internal use. */
> >
> > + struct rcu_head rcu;
> > +
> > char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> > refcount_t refcnt;
> > struct module *me; /* pointer to self */
> > struct nf_conntrack_expect_policy expect_policy[NF_CT_MAX_EXPECT_CLASSES];
> >
> > + refcount_t ct_refcnt;
>
> Why do we need two reference counts? I find this very confusing.
> Which refcount frees the structure? And can one refcount hit 0 while
> other one is still in use?
The existing refcnt tracks references from the control plane, ie.
rules that point to helper. The new ct_refcnt tracks references from
ct extension.
If the ruleset is flushed, then it is possible to unregister the
helper, otherwise EBUSY is reported. On the other hand, the ct_refcnt
tracks references by ct extension, eg. skb sitting in nfqueue with a
ct helper. The idea is to allow track the helper in memory so it does
not go away even in module is removed/userspace helper is destroyed.
Thus, helper can be removed anytime if ruleset does not use it, but
ct extension that still use the helper hold a reference on ct_refcnt
so it cannot be release. It is a two-level refcount strategy: The
control plane refcnt allows to remove the helper anytime, but the
dataplane refcnt can only be removed if control plane removed the
helper and no ct extension use it.
If I use a single refcnt (the existing one), then a packet sitting in
nfqueue can postpone the module removal of the helper indefinitely.
BTW, there is one single fix I can target to nf.git which is to
disallow userspace helpers in init_user_ns. Userspace helpers have no
netns support, I can post such small patch for inclusion. Still, the
unconfirmed ct race + nfqueue can theoretically still happen without
this series.
> > /* Function to call when data passes; return verdict, or -1 to
> > invalidate. */
> > - int (*help)(struct sk_buff *skb,
> > - unsigned int protoff,
> > - struct nf_conn *ct,
> > - enum ip_conntrack_info conntrackinfo);
> > + int __rcu (*help)(struct sk_buff *skb, unsigned int protoff,
> > + struct nf_conn *ct,
> > + enum ip_conntrack_info conntrackinfo);
> >
> > void (*destroy)(struct nf_conn *ct);
>
> Why is help RCU protected while other callbacks are not?
As you said above, this is the "dying flag".
> 'destroy' not being rcu protected implies that the helper module must
> remain in memory until after kfree_rcu has released the underlying
> storage anyway.
The only existing .destroy function is not moved in this series to
nf_conntrack_proto_gre.c which is part of the nf_conntrack module.
> If thats true, why do we need rcu head and kfree_rcu in the first place?
Because of userspace helpers, they do not depend on modules, they can
be removed via nfnetlink_cthelper anytime.
> module has to remain in memory until after last possible caller has
> called me->destroy(), no? If that is correct, then there is no need for
> dynamically allocated storage.
Maybe, but not related to ->destroy(), userspace helpers still are
there.
> > @@ -445,19 +432,18 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> > nf_ct_helper_count--;
> > mutex_unlock(&nf_ct_helper_mutex);
> >
> > + /* This helper is going away, disable it. */
> > + rcu_assign_pointer(me->help, NULL);
> > +
>
> OK, so this signals pending removal (refcnt can still be elevated) to
> prevent new packets/expectations from grabbing another reference.
> Correct? Is this a 'dying' flag or is there more to it?
Yes, this is a "dying flag".
> I looked at patched 'nf_conntrack_ftp_fini', but I don't see anything
> that spins/waits for completion of referencing entries.
Given the helper object is allocated dynamically, it will remain
around until last reference is dropped.
> How does ->destroy/to_nlattr/from_nlattr etc. work?
>
> I expected to find something that does a busywait until refcount has
> hit 0 to avoid any calls to the removed module.
>
> The existing conntracks still hold a pointer to struct
> nf_conntrack_helper, and its refcount can be elevated too, while
> function pointers (not help, but others) are stale.
.help is set to NULL.
.to_nlattr and .from_nlattr are disabled.
.destroy, I moved it to nf_conntrack with the intention that this
pointer is not stale.
> I suspect you need to move the function pointers to an 'op' sub-struct,
> so that it can be cleared via single rcu_assign_pointer(me->help_ops, NULL) ?
I think I cannot disable .destroy that way, it clears the GRE entries
which release the pptp mappings.
> But that still has one problem: if helper module is gone, how can you
> call the destructor?
In this series, the only .destroy callback resides in nf_conntrack.
> Maybe we need to accelerate pptp removal so the only user of destroy
> is removed?
Flagging it as deprecated is convenient for distributors to stop
compiling this, noone should be using this pptp in 2026 I think.
I'd rather see a more simple fix, but I am not sure this can be fixed
for all scenarios (sashiko mentioned also a skb could be sitting in
nf_defrag with a template conntrack with helper/timeout reference, so
nfqueue is no the only queue around).
Let me know, thanks for you comments.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath
2026-05-26 22:19 ` Pablo Neira Ayuso
@ 2026-05-27 13:39 ` Florian Westphal
0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2026-05-27 13:39 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> The existing refcnt tracks references from the control plane, ie.
> rules that point to helper. The new ct_refcnt tracks references from
> ct extension.
Hmm... AFAICS its only used by userspace helpers.
diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index de2f956abf34..31949f0c2755 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -33,7 +33,6 @@ struct nf_conntrack_helper {
struct hlist_node hnode; /* Internal use. */
char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
- refcount_t refcnt;
struct module *me; /* pointer to self */
const struct nf_conntrack_expect_policy *expect_policy;
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 17e971bd4c74..c3698885e4ba 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -92,10 +92,6 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
#endif
if (h != NULL && !try_module_get(h->me))
h = NULL;
- if (h != NULL && !refcount_inc_not_zero(&h->refcnt)) {
- module_put(h->me);
- h = NULL;
- }
rcu_read_unlock();
@@ -105,7 +101,6 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
{
- refcount_dec(&helper->refcnt);
module_put(helper->me);
}
EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
@@ -387,7 +382,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
}
}
}
- refcount_set(&me->refcnt, 1);
+
hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
nf_ct_helper_count++;
out:
... seems fine to me. Unload is blocked by module refcount.
This breaks with CONFIG_NF_CT_NETLINK_HELPER=m|y, of course.
But I don't see the need for a separate helper either.
AFAICS one could simply ignore the refcount on delete request:
Instead of -EBUSY, -> refcount_dec + unlink from data structure.
All other refcount holders need their own reference anyway.
memory release on 1 -> 0 transition.
If thats correct then we could use same refcount for both userspace
helpers and datapath.
But it looks like this refcnt is wrong in any case and should instead
live in struct nfnl_cthelper -- its not related to the (c-implemented)
helper backends.
> ct helper. The idea is to allow track the helper in memory so it does
> not go away even in module is removed/userspace helper is destroyed.
Yes, just so we are on the same page: that part makes sense to me.
> > > + /* This helper is going away, disable it. */
> > > + rcu_assign_pointer(me->help, NULL);
> > > +
> >
> > OK, so this signals pending removal (refcnt can still be elevated) to
> > prevent new packets/expectations from grabbing another reference.
> > Correct? Is this a 'dying' flag or is there more to it?
>
> Yes, this is a "dying flag".
Ok.
> > I looked at patched 'nf_conntrack_ftp_fini', but I don't see anything
> > that spins/waits for completion of referencing entries.
>
> Given the helper object is allocated dynamically, it will remain
> around until last reference is dropped.
Yes, I see that now. I was confused by other callbacks, esp.
->destroy, remaining initialised.
> .help is set to NULL.
> .to_nlattr and .from_nlattr are disabled.
> .destroy, I moved it to nf_conntrack with the intention that this
> pointer is not stale.
OK, so its in same module and followup patch could replace callback by a
direct call.
> > I suspect you need to move the function pointers to an 'op' sub-struct,
> > so that it can be cleared via single rcu_assign_pointer(me->help_ops, NULL) ?
>
> I think I cannot disable .destroy that way, it clears the GRE entries
> which release the pptp mappings.
Yes, I missed the fact that this is moved to the core.
> > Maybe we need to accelerate pptp removal so the only user of destroy
> > is removed?
>
> Flagging it as deprecated is convenient for distributors to stop
> compiling this, noone should be using this pptp in 2026 I think.
Agreed.
> I'd rather see a more simple fix, but I am not sure this can be fixed
> for all scenarios (sashiko mentioned also a skb could be sitting in
> nf_defrag with a template conntrack with helper/timeout reference, so
> nfqueue is no the only queue around).
Yes, agreed. I don't think there is a simple fix for this problem and
I think the direction taken here is sound.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH nf-next 6/6] netfilter: conntrack: revert ct extension genid infrastructure
2026-05-26 16:40 [PATCH nf-next 0/6] add refcount to ct timeout/helper Pablo Neira Ayuso
` (4 preceding siblings ...)
2026-05-26 16:40 ` [PATCH nf-next 5/6] netfilter: nf_conntrack_helper: add refcounting from datapath Pablo Neira Ayuso
@ 2026-05-26 16:40 ` Pablo Neira Ayuso
5 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2026-05-26 16:40 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw
This infrastructure is not used anymore after moving ct timeout and
helper to use datapath refcount to track object use.
Revert commit c56716c69ce1 ("netfilter: extensions: introduce extension
genid count") this patch disables all ct extensions (leading to NULL)
for unconfirmed conntracks, when this is only targeted at ct helper and
ct timeout. There is also codebase that dereferences the ct extension
without checking for NULL which could lead to crash.
This also reverts commit 2843fb69980b ("netfilter: conntrack: add
nf_ct_iterate_destroy") since the sledgehammer approach does not work
with unconfirmed conntrack that are flying to sit in nfqueue, or
elsewhere, then allowing potential UaF on reinjection.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack.h | 4 -
include/net/netfilter/nf_conntrack_extend.h | 12 ---
net/netfilter/nf_conntrack_core.c | 112 --------------------
net/netfilter/nf_conntrack_extend.c | 32 +-----
net/netfilter/nf_nat_core.c | 15 +--
net/netfilter/nfnetlink_cttimeout.c | 1 +
6 files changed, 4 insertions(+), 172 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index f75af8eb1cae..91c23bf42911 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -244,10 +244,6 @@ void nf_ct_iterate_cleanup(int (*iter)(struct nf_conn *i, void *data),
void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
const struct nf_ct_iter_data *iter_data);
-/* also set unconfirmed conntracks as dying. Only use in module exit path. */
-void nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data),
- void *data);
-
struct nf_conntrack_zone;
void nf_conntrack_free(struct nf_conn *ct);
diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 0b247248b032..fd5c4dbf72ca 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -38,7 +38,6 @@ enum nf_ct_ext_id {
struct nf_ct_ext {
u8 offset[NF_CT_EXT_NUM];
u8 len;
- unsigned int gen_id;
char data[] __aligned(8);
};
@@ -52,8 +51,6 @@ static inline bool nf_ct_ext_exist(const struct nf_conn *ct, u8 id)
return (ct->ext && __nf_ct_ext_exist(ct->ext, id));
}
-void *__nf_ct_ext_find(const struct nf_ct_ext *ext, u8 id);
-
static inline void *nf_ct_ext_find(const struct nf_conn *ct, u8 id)
{
struct nf_ct_ext *ext = ct->ext;
@@ -61,19 +58,10 @@ static inline void *nf_ct_ext_find(const struct nf_conn *ct, u8 id)
if (!ext || !__nf_ct_ext_exist(ext, id))
return NULL;
- if (unlikely(ext->gen_id))
- return __nf_ct_ext_find(ext, id);
-
return (void *)ct->ext + ct->ext->offset[id];
}
/* Add this type, returns pointer to data or NULL. */
void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp);
-/* ext genid. if ext->id != ext_genid, extensions cannot be used
- * anymore unless conntrack has CONFIRMED bit set.
- */
-extern atomic_t nf_conntrack_ext_genid;
-void nf_ct_ext_bump_genid(void);
-
#endif /* _NF_CONNTRACK_EXTEND_H */
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 493748f792de..b7ad399b79d9 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -833,33 +833,6 @@ static void __nf_conntrack_hash_insert(struct nf_conn *ct,
&nf_conntrack_hash[reply_hash]);
}
-static bool nf_ct_ext_valid_pre(const struct nf_ct_ext *ext)
-{
- /* if ext->gen_id is not equal to nf_conntrack_ext_genid, some extensions
- * may contain stale pointers to e.g. helper that has been removed.
- *
- * The helper can't clear this because the nf_conn object isn't in
- * any hash and synchronize_rcu() isn't enough because associated skb
- * might sit in a queue.
- */
- return !ext || ext->gen_id == atomic_read(&nf_conntrack_ext_genid);
-}
-
-static bool nf_ct_ext_valid_post(struct nf_ct_ext *ext)
-{
- if (!ext)
- return true;
-
- if (ext->gen_id != atomic_read(&nf_conntrack_ext_genid))
- return false;
-
- /* inserted into conntrack table, nf_ct_iterate_cleanup()
- * will find it. Disable nf_ct_ext_find() id check.
- */
- WRITE_ONCE(ext->gen_id, 0);
- return true;
-}
-
int
nf_conntrack_hash_check_insert(struct nf_conn *ct)
{
@@ -875,9 +848,6 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
zone = nf_ct_zone(ct);
- if (!nf_ct_ext_valid_pre(ct->ext))
- return -EAGAIN;
-
local_bh_disable();
do {
sequence = read_seqcount_begin(&nf_conntrack_generation);
@@ -911,18 +881,6 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
goto chaintoolong;
}
- /* If genid has changed, we can't insert anymore because ct
- * extensions could have stale pointers and nf_ct_iterate_destroy
- * might have completed its table scan already.
- *
- * Increment of the ext genid right after this check is fine:
- * nf_ct_iterate_destroy blocks until locks are released.
- */
- if (!nf_ct_ext_valid_post(ct->ext)) {
- err = -EAGAIN;
- goto out;
- }
-
smp_wmb();
/* The caller holds a reference to this object */
refcount_set(&ct->ct_general.use, 2);
@@ -1250,11 +1208,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
return NF_DROP;
}
- if (!nf_ct_ext_valid_pre(ct->ext)) {
- NF_CT_STAT_INC(net, insert_failed);
- goto dying;
- }
-
/* We have to check the DYING flag after unlink to prevent
* a race against nf_ct_get_next_corpse() possibly called from
* user context, else we insert an already 'dead' hash, blocking
@@ -1317,16 +1270,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
nf_conntrack_double_unlock(hash, reply_hash);
local_bh_enable();
- /* ext area is still valid (rcu read lock is held,
- * but will go out of scope soon, we need to remove
- * this conntrack again.
- */
- if (!nf_ct_ext_valid_post(ct->ext)) {
- nf_ct_kill(ct);
- NF_CT_STAT_INC_ATOMIC(net, drop);
- return NF_DROP;
- }
-
help = nfct_help(ct);
if (help && help->helper)
nf_conntrack_event_cache(IPCT_HELPER, ct);
@@ -2394,61 +2337,6 @@ void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
}
EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup_net);
-/**
- * nf_ct_iterate_destroy - destroy unconfirmed conntracks and iterate table
- * @iter: callback to invoke for each conntrack
- * @data: data to pass to @iter
- *
- * Like nf_ct_iterate_cleanup, but first marks conntracks on the
- * unconfirmed list as dying (so they will not be inserted into
- * main table).
- *
- * Can only be called in module exit path.
- */
-void
-nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
-{
- struct nf_ct_iter_data iter_data = {};
- struct net *net;
-
- down_read(&net_rwsem);
- for_each_net(net) {
- struct nf_conntrack_net *cnet = nf_ct_pernet(net);
-
- if (atomic_read(&cnet->count) == 0)
- continue;
- nf_queue_nf_hook_drop(net);
- }
- up_read(&net_rwsem);
-
- /* Need to wait for netns cleanup worker to finish, if its
- * running -- it might have deleted a net namespace from
- * the global list, so hook drop above might not have
- * affected all namespaces.
- */
- net_ns_barrier();
-
- /* 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();
-
- nf_ct_ext_bump_genid();
- iter_data.data = data;
- nf_ct_iterate_cleanup(iter, &iter_data);
-
- /* Another cpu might be in a rcu read section with
- * rcu protected pointer cleared in iter callback
- * or hidden via nf_ct_ext_bump_genid() above.
- *
- * Wait until those are done.
- */
- synchronize_rcu();
-}
-EXPORT_SYMBOL_GPL(nf_ct_iterate_destroy);
-
static int kill_all(struct nf_conn *i, void *data)
{
return 1;
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index dd62cc12e775..0da105e1ded9 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -27,8 +27,6 @@
#define NF_CT_EXT_PREALLOC 128u /* conntrack events are on by default */
-atomic_t nf_conntrack_ext_genid __read_mostly = ATOMIC_INIT(1);
-
static const u8 nf_ct_ext_type_len[NF_CT_EXT_NUM] = {
[NF_CT_EXT_HELPER] = sizeof(struct nf_conn_help),
#if IS_ENABLED(CONFIG_NF_NAT)
@@ -118,10 +116,8 @@ void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
if (!new)
return NULL;
- if (!ct->ext) {
+ if (!ct->ext)
memset(new->offset, 0, sizeof(new->offset));
- new->gen_id = atomic_read(&nf_conntrack_ext_genid);
- }
new->offset[id] = newoff;
new->len = newlen;
@@ -131,29 +127,3 @@ void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
return (void *)new + newoff;
}
EXPORT_SYMBOL(nf_ct_ext_add);
-
-/* Use nf_ct_ext_find wrapper. This is only useful for unconfirmed entries. */
-void *__nf_ct_ext_find(const struct nf_ct_ext *ext, u8 id)
-{
- unsigned int gen_id = atomic_read(&nf_conntrack_ext_genid);
- unsigned int this_id = READ_ONCE(ext->gen_id);
-
- if (!__nf_ct_ext_exist(ext, id))
- return NULL;
-
- if (this_id == 0 || ext->gen_id == gen_id)
- return (void *)ext + ext->offset[id];
-
- return NULL;
-}
-EXPORT_SYMBOL(__nf_ct_ext_find);
-
-void nf_ct_ext_bump_genid(void)
-{
- unsigned int value = atomic_inc_return(&nf_conntrack_ext_genid);
-
- if (value == UINT_MAX)
- atomic_set(&nf_conntrack_ext_genid, 1);
-
- msleep(HZ);
-}
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 74ec224ce0d6..e06ac58b8f79 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -969,20 +969,9 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb,
}
EXPORT_SYMBOL_GPL(nf_nat_inet_fn);
-struct nf_nat_proto_clean {
- u8 l3proto;
- u8 l4proto;
-};
-
/* kill conntracks with affected NAT section */
static int nf_nat_proto_remove(struct nf_conn *i, void *data)
{
- const struct nf_nat_proto_clean *clean = data;
-
- if ((clean->l3proto && nf_ct_l3num(i) != clean->l3proto) ||
- (clean->l4proto && nf_ct_protonum(i) != clean->l4proto))
- return 0;
-
return i->status & IPS_NAT_MASK ? 1 : 0;
}
@@ -1350,9 +1339,9 @@ static int __init nf_nat_init(void)
static void __exit nf_nat_cleanup(void)
{
- struct nf_nat_proto_clean clean = {};
+ struct nf_ct_iter_data iter_data = {};
- nf_ct_iterate_destroy(nf_nat_proto_clean, &clean);
+ nf_ct_iterate_cleanup(nf_nat_proto_clean, &iter_data);
nf_ct_helper_expectfn_unregister(&follow_master_nat);
RCU_INIT_POINTER(nf_nat_hook, NULL);
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 8efda53f94eb..739a8461ff9f 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -677,6 +677,7 @@ static void __exit cttimeout_exit(void)
unregister_pernet_subsys(&cttimeout_ops);
RCU_INIT_POINTER(nf_ct_timeout_hook, NULL);
+ synchronize_net();
nf_ct_untimeout(NULL, NULL);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 10+ messages in thread