[NETFILTER]: nf_conntrack: fix helper module unload races
When a helper module is unloaded all conntracks refering to it have their
helper pointer NULLed out, leading to lots of races. In most places this
can be fixed by proper use of RCU (they do already check for != NULL,
but in a racy way), additionally nf_conntrack_expect_related needs to
bail out when no helper is present.
Also remove two paranoid BUG_ONs in nf_conntrack_proto_gre that are racy
and not worth fixing.
Signed-off-by: Patrick McHarrdy <kaber@trash.net>
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index fd62a41..6dc72a8 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -133,6 +133,7 @@
struct nf_conn *ct;
enum ip_conntrack_info ctinfo;
struct nf_conn_help *help;
+ struct nf_conntrack_helper *helper;
/* This is where we call the helper: as the packet goes out. */
ct = nf_ct_get(*pskb, &ctinfo);
@@ -140,12 +141,14 @@
return NF_ACCEPT;
help = nfct_help(ct);
- if (!help || !help->helper)
+ if (!help)
return NF_ACCEPT;
-
- return help->helper->help(pskb,
- skb_network_offset(*pskb) + ip_hdrlen(*pskb),
- ct, ctinfo);
+ /* rcu_read_lock()ed by nf_hook_slow */
+ helper = rcu_dereference(help->helper);
+ if (!helper)
+ return NF_ACCEPT;
+ return helper->help(pskb, skb_network_offset(*pskb) + ip_hdrlen(*pskb),
+ ct, ctinfo);
}
static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index dc442fb..1b1797f 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -160,6 +160,7 @@
{
struct nf_conn *ct;
struct nf_conn_help *help;
+ struct nf_conntrack_helper *helper;
enum ip_conntrack_info ctinfo;
unsigned int ret, protoff;
unsigned int extoff = (u8 *)(ipv6_hdr(*pskb) + 1) - (*pskb)->data;
@@ -172,7 +173,11 @@
goto out;
help = nfct_help(ct);
- if (!help || !help->helper)
+ if (!help)
+ goto out;
+ /* rcu_read_lock()ed by nf_hook_slow */
+ helper = rcu_dereference(help->helper);
+ if (!helper)
goto out;
protoff = nf_ct_ipv6_skip_exthdr(*pskb, extoff, &pnum,
@@ -182,7 +187,7 @@
return NF_ACCEPT;
}
- ret = help->helper->help(pskb, protoff, ct, ctinfo);
+ ret = helper->help(pskb, protoff, ct, ctinfo);
if (ret != NF_ACCEPT)
return ret;
out:
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 483e927..7a15e30 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -350,9 +350,15 @@
{
struct nf_conn *ct = (void *)ul_conntrack;
struct nf_conn_help *help = nfct_help(ct);
+ struct nf_conntrack_helper *helper;
- if (help && help->helper && help->helper->destroy)
- help->helper->destroy(ct);
+ if (help) {
+ rcu_read_lock();
+ helper = rcu_dereference(help->helper);
+ if (helper && helper->destroy)
+ helper->destroy(ct);
+ rcu_read_unlock();
+ }
write_lock_bh(&nf_conntrack_lock);
/* Inside lock so preempt is disabled on module removal path.
@@ -661,6 +667,7 @@
unsigned int dataoff)
{
struct nf_conn *conntrack;
+ struct nf_conn_help *help;
struct nf_conntrack_tuple repl_tuple;
struct nf_conntrack_expect *exp;
u_int32_t features = 0;
@@ -691,6 +698,7 @@
write_lock_bh(&nf_conntrack_lock);
exp = find_expectation(tuple);
+ help = nfct_help(conntrack);
if (exp) {
DEBUGP("conntrack: expectation arrives ct=%p exp=%p\n",
conntrack, exp);
@@ -698,7 +706,7 @@
__set_bit(IPS_EXPECTED_BIT, &conntrack->status);
conntrack->master = exp->master;
if (exp->helper)
- nfct_help(conntrack)->helper = exp->helper;
+ rcu_assign_pointer(help->helper, exp->helper);
#ifdef CONFIG_NF_CONNTRACK_MARK
conntrack->mark = exp->master->mark;
#endif
@@ -708,10 +716,11 @@
nf_conntrack_get(&conntrack->master->ct_general);
NF_CT_STAT_INC(expect_new);
} else {
- struct nf_conn_help *help = nfct_help(conntrack);
-
- if (help)
- help->helper = __nf_ct_helper_find(&repl_tuple);
+ if (help) {
+ /* not in hash table yet, so not strictly necessary */
+ rcu_assign_pointer(help->helper,
+ __nf_ct_helper_find(&repl_tuple));
+ }
NF_CT_STAT_INC(new);
}
@@ -893,7 +902,8 @@
helper = __nf_ct_helper_find(newreply);
if (helper)
memset(&help->help, 0, sizeof(help->help));
- help->helper = helper;
+ /* not in hash table yet, so not strictly necessary */
+ rcu_assign_pointer(help->helper, helper);
}
write_unlock_bh(&nf_conntrack_lock);
}
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 117cbfd..504fb6c 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -337,6 +337,10 @@
NF_CT_ASSERT(master_help);
write_lock_bh(&nf_conntrack_lock);
+ if (!master_help->helper) {
+ ret = -ESHUTDOWN;
+ goto out;
+ }
list_for_each_entry(i, &nf_conntrack_expect_list, list) {
if (expect_matches(i, expect)) {
/* Refresh timer: if it's dying, ignore.. */
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 0743be4..f868b7f 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -93,7 +93,7 @@
if (help && help->helper == me) {
nf_conntrack_event(IPCT_HELPER, ct);
- help->helper = NULL;
+ rcu_assign_pointer(help->helper, NULL);
}
return 0;
}
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index d6d39e2..3f73327 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -171,21 +171,29 @@
{
struct nfattr *nest_helper;
const struct nf_conn_help *help = nfct_help(ct);
+ struct nf_conntrack_helper *helper;
- if (!help || !help->helper)
+ if (!help)
return 0;
- nest_helper = NFA_NEST(skb, CTA_HELP);
- NFA_PUT(skb, CTA_HELP_NAME, strlen(help->helper->name), help->helper->name);
+ rcu_read_lock();
+ helper = rcu_dereference(help->helper);
+ if (!helper)
+ goto out;
- if (help->helper->to_nfattr)
- help->helper->to_nfattr(skb, ct);
+ nest_helper = NFA_NEST(skb, CTA_HELP);
+ NFA_PUT(skb, CTA_HELP_NAME, strlen(helper->name), helper->name);
+
+ if (helper->to_nfattr)
+ helper->to_nfattr(skb, ct);
NFA_NEST_END(skb, nest_helper);
-
+out:
+ rcu_read_unlock();
return 0;
nfattr_failure:
+ rcu_read_unlock();
return -1;
}
@@ -842,7 +850,7 @@
if (help && help->helper) {
/* we had a helper before ... */
nf_ct_remove_expectations(ct);
- help->helper = NULL;
+ rcu_assign_pointer(help->helper, NULL);
}
return 0;
@@ -866,7 +874,7 @@
/* need to zero data of old helper */
memset(&help->help, 0, sizeof(help->help));
- help->helper = helper;
+ rcu_assign_pointer(help->helper, helper);
return 0;
}
@@ -950,6 +958,7 @@
struct nf_conn *ct;
int err = -EINVAL;
struct nf_conn_help *help;
+ struct nf_conntrack_helper *helper = NULL;
ct = nf_conntrack_alloc(otuple, rtuple);
if (ct == NULL || IS_ERR(ct))
@@ -980,14 +989,17 @@
#endif
help = nfct_help(ct);
- if (help)
- help->helper = nf_ct_helper_find_get(rtuple);
+ if (help) {
+ helper = nf_ct_helper_find_get(rtuple);
+ /* not in hash table yet so not strictly necessary */
+ rcu_assign_pointer(help->helper, helper);
+ }
add_timer(&ct->timeout);
nf_conntrack_hash_insert(ct);
- if (help && help->helper)
- nf_ct_helper_put(help->helper);
+ if (helper)
+ nf_ct_helper_put(helper);
return 0;
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 5434472..339c397 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -100,7 +100,6 @@
struct nf_conn_help *help = nfct_help(ct);
struct nf_ct_gre_keymap **kmp, *km;
- BUG_ON(strcmp(help->helper->name, "pptp"));
kmp = &help->help.ct_pptp_info.keymap[dir];
if (*kmp) {
/* check whether it's a retransmission */
@@ -137,7 +136,6 @@
enum ip_conntrack_dir dir;
DEBUGP("entering for ct %p\n", ct);
- BUG_ON(strcmp(help->helper->name, "pptp"));
write_lock_bh(&nf_ct_gre_lock);
for (dir = IP_CT_DIR_ORIGINAL; dir < IP_CT_DIR_MAX; dir++) {