[PATCH nf 1/3] netfilter: conntrack: fix crash on timeout object removal

[PATCH nf 1/3] netfilter: conntrack: fix crash on timeout object removal

[Pablo Neira Ayuso]

The object and module refcounts are updated for each conntrack template,
however, if we delete the iptables rules and we flush the timeout
database, we may end up with invalid references to timeout object that
are just gone.

Resolve this race by setting the timeout reference to NULL when the
custom timeout entry is removed from our base. This patch requires some
RCU trickery to ensure safe pointer handling.

This handling is similer to what we already do with conntrack helpers,
the idea is to avoid bumping the timeout object reference counter from
the packet path to avoid the cost of atomic ops.

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_timeout.h | 25 ++++++++++++++++-----
 net/netfilter/nf_conntrack_core.c            |  9 +++++---
 net/netfilter/nfnetlink_cttimeout.c          | 33 ++++++++++++++++++++++++++++
 net/netfilter/xt_CT.c                        |  4 +++-
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
index 6230871..f72be38 100644
--- a/include/net/netfilter/nf_conntrack_timeout.h
+++ b/include/net/netfilter/nf_conntrack_timeout.h
@@ -20,10 +20,20 @@ struct ctnl_timeout {
 };
 
 struct nf_conn_timeout {
-	struct ctnl_timeout	*timeout;
+	struct ctnl_timeout __rcu *timeout;
 };
 
-#define NF_CT_TIMEOUT_EXT_DATA(__t) (unsigned int *) &((__t)->timeout->data)
+static inline unsigned int *
+nf_ct_timeout_data(struct nf_conn_timeout *t)
+{
+	struct ctnl_timeout *timeout;
+
+	timeout = rcu_dereference(t->timeout);
+	if (timeout == NULL)
+		return NULL;
+
+	return (unsigned int *)timeout->data;
+}
 
 static inline
 struct nf_conn_timeout *nf_ct_timeout_find(const struct nf_conn *ct)
@@ -47,7 +57,7 @@ struct nf_conn_timeout *nf_ct_timeout_ext_add(struct nf_conn *ct,
 	if (timeout_ext == NULL)
 		return NULL;
 
-	timeout_ext->timeout = timeout;
+	rcu_assign_pointer(timeout_ext->timeout, timeout);
 
 	return timeout_ext;
 #else
@@ -64,10 +74,13 @@ nf_ct_timeout_lookup(struct net *net, struct nf_conn *ct,
 	unsigned int *timeouts;
 
 	timeout_ext = nf_ct_timeout_find(ct);
-	if (timeout_ext)
-		timeouts = NF_CT_TIMEOUT_EXT_DATA(timeout_ext);
-	else
+	if (timeout_ext) {
+		timeouts = nf_ct_timeout_data(timeout_ext);
+		if (unlikely(!timeouts))
+			timeouts = l4proto->get_timeouts(net);
+	} else {
 		timeouts = l4proto->get_timeouts(net);
+	}
 
 	return timeouts;
 #else
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 09d1d19..6fc89d8 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -940,10 +940,13 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 	}
 
 	timeout_ext = tmpl ? nf_ct_timeout_find(tmpl) : NULL;
-	if (timeout_ext)
-		timeouts = NF_CT_TIMEOUT_EXT_DATA(timeout_ext);
-	else
+	if (timeout_ext) {
+		timeouts = nf_ct_timeout_data(timeout_ext);
+		if (unlikely(!timeouts))
+			timeouts = l4proto->get_timeouts(net);
+	} else {
 		timeouts = l4proto->get_timeouts(net);
+	}
 
 	if (!l4proto->new(ct, skb, dataoff, timeouts)) {
 		nf_conntrack_free(ct);
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 476accd..138cd0a 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -291,6 +291,34 @@ cttimeout_get_timeout(struct sock *ctnl, struct sk_buff *skb,
 	return ret;
 }
 
+static void untimeout(struct nf_conntrack_tuple_hash *i,
+		      struct ctnl_timeout *timeout)
+{
+	struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(i);
+	struct nf_conn_timeout *timeout_ext = nf_ct_timeout_find(ct);
+
+	if (timeout_ext && (!timeout || timeout_ext->timeout == timeout))
+                RCU_INIT_POINTER(timeout_ext->timeout, NULL);
+}
+
+static void ctnl_untimeout(struct ctnl_timeout *timeout)
+{
+	struct nf_conntrack_tuple_hash *h;
+	const struct hlist_nulls_node *nn;
+	int i;
+
+	local_bh_disable();
+	for (i = 0; i < init_net.ct.htable_size; i++) {
+		spin_lock(&nf_conntrack_locks[i % CONNTRACK_LOCKS]);
+		if (i < init_net.ct.htable_size) {
+			hlist_nulls_for_each_entry(h, nn, &init_net.ct.hash[i], hnnode)
+				untimeout(h, timeout);
+		}
+		spin_unlock(&nf_conntrack_locks[i % CONNTRACK_LOCKS]);
+	}
+	local_bh_enable();
+}
+
 /* try to delete object, fail if it is still in use. */
 static int ctnl_timeout_try_del(struct ctnl_timeout *timeout)
 {
@@ -301,6 +329,7 @@ static int ctnl_timeout_try_del(struct ctnl_timeout *timeout)
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&timeout->head);
 		nf_ct_l4proto_put(timeout->l4proto);
+		ctnl_untimeout(timeout);
 		kfree_rcu(timeout, rcu_head);
 	} else {
 		/* still in use, restore reference counter. */
@@ -567,6 +596,10 @@ static void __exit cttimeout_exit(void)
 	pr_info("cttimeout: unregistering from nfnetlink.\n");
 
 	nfnetlink_subsys_unregister(&cttimeout_subsys);
+
+	/* Make sure no conntrack objects refer to custom timeouts anymore. */
+	ctnl_untimeout(NULL);
+
 	list_for_each_entry_safe(cur, tmp, &cttimeout_list, head) {
 		list_del_rcu(&cur->head);
 		/* We are sure that our objects have no clients at this point,
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index faf32d8..b43bf13 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -318,8 +318,10 @@ static void xt_ct_destroy_timeout(struct nf_conn *ct)
 
 	if (timeout_put) {
 		timeout_ext = nf_ct_timeout_find(ct);
-		if (timeout_ext)
+		if (timeout_ext) {
 			timeout_put(timeout_ext->timeout);
+			RCU_INIT_POINTER(timeout_ext->timeout, NULL);
+		}
 	}
 	rcu_read_unlock();
 #endif
-- 
2.1.4

[PATCH nf 2/3] nfnetlink_cttimeout: add rcu_barrier() on module removal

[Pablo Neira Ayuso]

Make sure kfree_rcu() released objects before leaving the module removal
exit path.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_cttimeout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 138cd0a..e90e4fc 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -612,6 +612,7 @@ static void __exit cttimeout_exit(void)
 	RCU_INIT_POINTER(nf_ct_timeout_find_get_hook, NULL);
 	RCU_INIT_POINTER(nf_ct_timeout_put_hook, NULL);
 #endif /* CONFIG_NF_CONNTRACK_TIMEOUT */
+	rcu_barrier();
 }
 
 module_init(cttimeout_init);
-- 
2.1.4

[PATCH nf 3/3] netfilter: xt_CT: don't put back reference to timeout policy object

[Pablo Neira Ayuso]

On success, this shouldn't put back the timeout policy object, otherwise
we may have module refcount overflow and we allow deletion of timeout
that are still in use.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index b43bf13..e8ac8e0 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -171,6 +171,9 @@ xt_ct_set_timeout(struct nf_conn *ct, const struct xt_tgchk_param *par,
 	if (timeout_ext == NULL)
 		ret = -ENOMEM;
 
+	rcu_read_unlock();
+	return 0;
+
 err_put_timeout:
 	__xt_ct_tg_timeout_put(timeout);
 out:
-- 
2.1.4

Re: [PATCH nf 1/3] netfilter: conntrack: fix crash on timeout object removal

[kbuild test robot]

Hi Pablo,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> net/netfilter/nf_conntrack_core.c:956:54: sparse: incorrect type in argument 2 (different address spaces)
   net/netfilter/nf_conntrack_core.c:956:54:    expected struct ctnl_timeout *timeout
   net/netfilter/nf_conntrack_core.c:956:54:    got struct ctnl_timeout [noderef] <asn:4>*timeout
   net/netfilter/nf_conntrack_core.c:1463:9: sparse: incompatible types in comparison expression (different address spaces)
   net/netfilter/nf_conntrack_core.c:1468:9: sparse: incompatible types in comparison expression (different address spaces)
   net/netfilter/nf_conntrack_core.c:1744:9: sparse: incompatible types in comparison expression (different address spaces)
   net/netfilter/nf_conntrack_core.c:1745:9: sparse: incompatible types in comparison expression (different address spaces)

vim +956 net/netfilter/nf_conntrack_core.c

60b5f8f7 Pablo Neira Ayuso       2012-03-23  940  	timeout_ext = tmpl ? nf_ct_timeout_find(tmpl) : NULL;
2d28d14a Pablo Neira Ayuso       2015-10-05  941  	if (timeout_ext) {
2d28d14a Pablo Neira Ayuso       2015-10-05  942  		timeouts = nf_ct_timeout_data(timeout_ext);
2d28d14a Pablo Neira Ayuso       2015-10-05  943  		if (unlikely(!timeouts))
2d28d14a Pablo Neira Ayuso       2015-10-05  944  			timeouts = l4proto->get_timeouts(net);
2d28d14a Pablo Neira Ayuso       2015-10-05  945  	} else {
60b5f8f7 Pablo Neira Ayuso       2012-03-23  946  		timeouts = l4proto->get_timeouts(net);
2d28d14a Pablo Neira Ayuso       2015-10-05  947  	}
60b5f8f7 Pablo Neira Ayuso       2012-03-23  948  
2c8503f5 Pablo Neira Ayuso       2012-02-28  949  	if (!l4proto->new(ct, skb, dataoff, timeouts)) {
c88130bc Patrick McHardy         2008-01-31  950  		nf_conntrack_free(ct);
0d53778e Patrick McHardy         2007-07-07  951  		pr_debug("init conntrack: can't track with proto module\n");
9fb9cbb1 Yasuyuki Kozakai        2005-11-09  952  		return NULL;
9fb9cbb1 Yasuyuki Kozakai        2005-11-09  953  	}
9fb9cbb1 Yasuyuki Kozakai        2005-11-09  954  
60b5f8f7 Pablo Neira Ayuso       2012-03-23  955  	if (timeout_ext)
60b5f8f7 Pablo Neira Ayuso       2012-03-23 @956  		nf_ct_timeout_ext_add(ct, timeout_ext->timeout, GFP_ATOMIC);
60b5f8f7 Pablo Neira Ayuso       2012-03-23  957  
58401572 Krzysztof Piotr Oledzki 2008-07-21  958  	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
a992ca2a Pablo Neira Ayuso       2011-01-19  959  	nf_ct_tstamp_ext_add(ct, GFP_ATOMIC);
c539f017 Florian Westphal        2013-01-11  960  	nf_ct_labels_ext_add(ct);
b2a15a60 Patrick McHardy         2010-02-03  961  
b2a15a60 Patrick McHardy         2010-02-03  962  	ecache = tmpl ? nf_ct_ecache_find(tmpl) : NULL;
b2a15a60 Patrick McHardy         2010-02-03  963  	nf_ct_ecache_ext_add(ct, ecache ? ecache->ctmask : 0,
b2a15a60 Patrick McHardy         2010-02-03  964  				 ecache ? ecache->expmask : 0,

:::::: The code at line 956 was first introduced by commit
:::::: 60b5f8f745739a4789395648595ed31ede582448 netfilter: nf_conntrack: permanently attach timeout policy to conntrack

:::::: TO: Pablo Neira Ayuso <pablo@netfilter.org>
:::::: CC: Pablo Neira Ayuso <pablo@netfilter.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH nf 1/3] netfilter: conntrack: fix crash on timeout object removal

[Pablo Neira Ayuso]

On Mon, Oct 05, 2015 at 11:08:57PM +0800, kbuild test robot wrote:
> Hi Pablo,
> 
[...]
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> net/netfilter/nf_conntrack_core.c:956:54: sparse: incorrect type in argument 2 (different address spaces)
>    net/netfilter/nf_conntrack_core.c:956:54:    expected struct ctnl_timeout *timeout
>    net/netfilter/nf_conntrack_core.c:956:54:    got struct ctnl_timeout [noderef] <asn:4>*timeout
>    net/netfilter/nf_conntrack_core.c:1463:9: sparse: incompatible types in comparison expression (different address spaces)
>    net/netfilter/nf_conntrack_core.c:1468:9: sparse: incompatible types in comparison expression (different address spaces)
>    net/netfilter/nf_conntrack_core.c:1744:9: sparse: incompatible types in comparison expression (different address spaces)
>    net/netfilter/nf_conntrack_core.c:1745:9: sparse: incompatible types in comparison expression (different address spaces)
> 
> vim +956 net/netfilter/nf_conntrack_core.c
[...]
>
> 60b5f8f7 Pablo Neira Ayuso       2012-03-23  955  	if (timeout_ext)
> 60b5f8f7 Pablo Neira Ayuso       2012-03-23 @956  		nf_ct_timeout_ext_add(ct, timeout_ext->timeout, GFP_ATOMIC);

Fixed here through before pushing out this patch:

        nf_ct_timeout_ext_add(ct, rcu_deference(timeout_ext->timeout), GFP_ATOMIC);

I can't see any sparse warning anymore with the change above.

Thanks!