Patchwork netfilter: conntrack: death_by_timeout() fix

login
register
mail settings
Submitter Eric Dumazet
Date June 18, 2009, 10:46 p.m.
Message ID <4A3AC3B3.2030002@gmail.com>
Download mbox | patch
Permalink /patch/28885/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - June 18, 2009, 10:46 p.m.
Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> In my own analysis, I found death_by_timeout() might be problematic,
>> with RCU and lockless lookups.
>>
>> static void death_by_timeout(unsigned long ul_conntrack)
>> {
>>         struct nf_conn *ct = (void *)ul_conntrack;
>>
>>         if (!test_bit(IPS_DYING_BIT, &ct->status) &&
>>             unlikely(nf_conntrack_event(IPCT_DESTROY, ct) < 0)) {
>>                 /* destroy event was not delivered */
>>                 nf_ct_delete_from_lists(ct);
>> << HERE >>
>>
>>                 nf_ct_insert_dying_list(ct);
>>                 return;
>>         }
>>         set_bit(IPS_DYING_BIT, &ct->status);
>>         nf_ct_delete_from_lists(ct);
>>         nf_ct_put(ct);
>> }
>>
>>
>> We delete ct from a list and insert it in a new list.
>>
>> I believe a reader could "*catch*" ct while doing a lookup and miss
>> the end
>> of its chain. (nulls algo check the null value at the end of lookup
>> and can
>> decide to restart the lookup if the null value is not the expected one)
>>
>> We need to change nf_conntrack_init_net() and use a different "null"
>> value,
>> guaranteed not being used in regular lists
> 
> Good catch. This is a new bug, but it shouldn't matter in this case
> since nf_conntrack_event() can't fail unless you have a userspace
> listener that makes use of reliable delivery, which I think hasn't
> even been released yet.
> 
>> Patch follows :
> 
> Looks good. If you send me a Signed-off-by: I'll already apply it.

Sure, here it is.

Thank you

[PATCH] netfilter: conntrack: death_by_timeout() fix

death_by_timeout() might delete a conntrack from hash list
and insert it in dying list.

 nf_ct_delete_from_lists(ct);
 nf_ct_insert_dying_list(ct);

I believe a (lockless) reader could *catch* ct while doing a lookup 
and miss the end of its chain.
(nulls lookup algo must check the null value at the end of lookup and
should restart if the null value is not the expected one.
cf Documentation/RCU/rculist_nulls.txt for details)

We need to change nf_conntrack_init_net() and use a different "null" value,
guaranteed not being used in regular lists. Choose very large values, since
hash table uses [0..size-1] null values.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
Acked-by: Patrick McHardy <kaber@trash.net>
---


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy - June 19, 2009, 11:15 a.m.
Eric Dumazet wrote:
> [PATCH] netfilter: conntrack: death_by_timeout() fix
> 
> death_by_timeout() might delete a conntrack from hash list
> and insert it in dying list.
> 
>  nf_ct_delete_from_lists(ct);
>  nf_ct_insert_dying_list(ct);
> 
> I believe a (lockless) reader could *catch* ct while doing a lookup 
> and miss the end of its chain.
> (nulls lookup algo must check the null value at the end of lookup and
> should restart if the null value is not the expected one.
> cf Documentation/RCU/rculist_nulls.txt for details)
> 
> We need to change nf_conntrack_init_net() and use a different "null" value,
> guaranteed not being used in regular lists. Choose very large values, since
> hash table uses [0..size-1] null values.

Applied, thanks Eric.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5f72b94..5276a2d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1267,13 +1267,19 @@  err_cache:
 	return ret;
 }
 
+/*
+ * We need to use special "null" values, not used in hash table
+ */
+#define UNCONFIRMED_NULLS_VAL	((1<<30)+0)
+#define DYING_NULLS_VAL		((1<<30)+1)
+
 static int nf_conntrack_init_net(struct net *net)
 {
 	int ret;
 
 	atomic_set(&net->ct.count, 0);
-	INIT_HLIST_NULLS_HEAD(&net->ct.unconfirmed, 0);
-	INIT_HLIST_NULLS_HEAD(&net->ct.dying, 0);
+	INIT_HLIST_NULLS_HEAD(&net->ct.unconfirmed, UNCONFIRMED_NULLS_VAL);
+	INIT_HLIST_NULLS_HEAD(&net->ct.dying, DYING_NULLS_VAL);
 	net->ct.stat = alloc_percpu(struct ip_conntrack_stat);
 	if (!net->ct.stat) {
 		ret = -ENOMEM;