| Message ID | 20260514143016.874811-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [nf,v2] netfilter: conntrack: add dead flag to helpers | expand |
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() ?
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.
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?
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.
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?
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 :-)
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?
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?
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.
On Fri, May 15, 2026 at 02:26:31PM +0200, Florian Westphal wrote: > 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? Just to close this previous thread now that we're mixing things here: No issue with unconfirmed conntrack that gets enqueued in nfqueue since c56716c69ce1 ("netfilter: extensions: introduce extension genid count") handles the unconfirmed ct sitting in nfqueue or elsewhere. > > > > 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? Are you referring to this specifically? "This isn't an issue introduced by this patch, but does destroy_gre_conntrack() safely cast the master connection's helper data? If an unprivileged attacker with CAP_NET_ADMIN uses ctnetlink to create a GRE expectation linked to a master connection that uses a different helper (like FTP) or no helper at all, the function blindly casts the helper data to struct nf_ct_pptp_master. Since FTP helper data contains attacker-controllable sequence numbers, could this cause list_del_rcu() and kfree_rcu() to operate on attacker-controlled addresses, leading to an arbitrary free or list corruption?" I don't think that is really possible via ctnetlink: - ctnetlink_alloc_expect() currently checks if nfct_help() exists, otherwise it reports EOPNOTSUPP. - ctnetlink_alloc_expect() uses the existing helper in the master conntrack to create the expectation. - exp->assign_helper attaches a helper to the expected conntrack. But that is a basically a new different conntrack. > 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. I don't see how yet this access to mismatching helper data type area can happen. > Callers must handle NULL return value everywhere (wrong helper type, > helper invalidated, etc). Yes, callers must handle NULL in nfct_help_data() because the helper extension might become stale as per c56716c69ce1. > 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? Yes, I think .destroy should be called from unhelp() path otherwise pptp info will remain in the GRE keymap. > Ideally we could get rid of ->destroy, but that would require > permanent removal of pptp. This is the only user of ->destroy, I think the pptp helper is a candidate to be deprecated.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Just to close this previous thread now that we're mixing things here: > No issue with unconfirmed conntrack that gets enqueued in nfqueue > since > > c56716c69ce1 ("netfilter: extensions: introduce extension genid count") > > handles the unconfirmed ct sitting in nfqueue or elsewhere. Sorry :-( There are so many things going on that I am losing track of some stuff. Yes, I think its fine but I need/want to go over this in detail. Not sure I can though, I am tied up with non-software related things tomorrow. I hope I can look into the conntrack stuff on Wed or Thursday. > Are you referring to this specifically? > > "This isn't an issue introduced by this patch, but does destroy_gre_conntrack() > safely cast the master connection's helper data? > If an unprivileged attacker with CAP_NET_ADMIN uses ctnetlink to create a GRE > expectation linked to a master connection that uses a different helper > (like FTP) or no helper at all, the function blindly casts the helper data > to struct nf_ct_pptp_master. > Since FTP helper data contains attacker-controllable sequence numbers, could > this cause list_del_rcu() and kfree_rcu() to operate on attacker-controlled > addresses, leading to an arbitrary free or list corruption?" Yes, that is waht I was referring to. > I don't think that is really possible via ctnetlink: > > - ctnetlink_alloc_expect() currently checks if nfct_help() exists, > otherwise it reports EOPNOTSUPP. > > - ctnetlink_alloc_expect() uses the existing helper in the master > conntrack to create the expectation. > > - exp->assign_helper attaches a helper to the expected conntrack. > But that is a basically a new different conntrack. Thank you for these pointers. The bug would only manifest if you can make a GRE conntrack entry whose ct->master is tracked by FTP for instance. > > 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. > > I don't see how yet this access to mismatching helper data type area > can happen. Great :-) > > Callers must handle NULL return value everywhere (wrong helper type, > > helper invalidated, etc). > > Yes, callers must handle NULL in nfct_help_data() because the helper > extension might become stale as per c56716c69ce1. I will work on something for nf-next to handle this. I spent yesterday looking at helper infra and there is some code that can be axed, leftovers from autoassign days. I'll work on that. > > 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? > > Yes, I think .destroy should be called from unhelp() path otherwise > pptp info will remain in the GRE keymap. I'll send/apply a patch to do that this week and will include it in PR. > > Ideally we could get rid of ->destroy, but that would require > > permanent removal of pptp. > > This is the only user of ->destroy, I think the pptp helper is a > candidate to be deprecated. Yes, its the only user of this destructor. I'll make a patch for nf-next to add deprecation tags (pr_warn_once + Kconfig). Thanks Pablo!
On Mon, May 18, 2026 at 11:49:55PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Just to close this previous thread now that we're mixing things here: > > No issue with unconfirmed conntrack that gets enqueued in nfqueue > > since > > > > c56716c69ce1 ("netfilter: extensions: introduce extension genid count") > > > > handles the unconfirmed ct sitting in nfqueue or elsewhere. > > Sorry :-( There are so many things going on that I am losing track > of some stuff. I can help collect patches in this round too. > Yes, I think its fine but I need/want to go over this in detail. Not > sure I can though, I am tied up with non-software related things > tomorrow. I hope I can look into the conntrack stuff on Wed or > Thursday. > > > Are you referring to this specifically? > > > > "This isn't an issue introduced by this patch, but does destroy_gre_conntrack() > > safely cast the master connection's helper data? > > If an unprivileged attacker with CAP_NET_ADMIN uses ctnetlink to create a GRE > > expectation linked to a master connection that uses a different helper > > (like FTP) or no helper at all, the function blindly casts the helper data > > to struct nf_ct_pptp_master. > > Since FTP helper data contains attacker-controllable sequence numbers, could > > this cause list_del_rcu() and kfree_rcu() to operate on attacker-controlled > > addresses, leading to an arbitrary free or list corruption?" > > Yes, that is waht I was referring to. > > > I don't think that is really possible via ctnetlink: > > > > - ctnetlink_alloc_expect() currently checks if nfct_help() exists, > > otherwise it reports EOPNOTSUPP. > > > > - ctnetlink_alloc_expect() uses the existing helper in the master > > conntrack to create the expectation. > > > > - exp->assign_helper attaches a helper to the expected conntrack. > > But that is a basically a new different conntrack. > > Thank you for these pointers. The bug would only manifest if you can > make a GRE conntrack entry whose ct->master is tracked by FTP for instance. > > > > 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. > > > > I don't see how yet this access to mismatching helper data type area > > can happen. > > Great :-) > > > > Callers must handle NULL return value everywhere (wrong helper type, > > > helper invalidated, etc). > > > > Yes, callers must handle NULL in nfct_help_data() because the helper > > extension might become stale as per c56716c69ce1. > > I will work on something for nf-next to handle this. I spent yesterday > looking at helper infra and there is some code that can be axed, > leftovers from autoassign days. I'll work on that. Posted a patch for this, it needs another spin. https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260518191232.1053294-3-pablo@netfilter.org/ I will take care of this. > > > 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? > > > > Yes, I think .destroy should be called from unhelp() path otherwise > > pptp info will remain in the GRE keymap. > > I'll send/apply a patch to do that this week and will include it in PR. OK. > > > Ideally we could get rid of ->destroy, but that would require > > > permanent removal of pptp. > > > > This is the only user of ->destroy, I think the pptp helper is a > > candidate to be deprecated. > > Yes, its the only user of this destructor. I'll make a patch for > nf-next to add deprecation tags (pr_warn_once + Kconfig). OK. Would you just respin of the GRE kmap issues that sashiko reported? Otherwise I can pick up on it.
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);
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(-)