Thread (33 messages) 33 messages, 8 authors, 2014-02-03

Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

From: Eric Dumazet <hidden>
Date: 2014-01-09 15:23:56
Also in: lkml, netfilter-devel

On Thu, 2014-01-09 at 09:24 +0400, Andrew Vagin wrote:
I have one more question. Looks like I found another problem.

init_conntrack:
        hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode,
                      &net->ct.unconfirmed);

__nf_conntrack_hash_insert:
	hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode,
		   &net->ct.hash[hash]);

We use one hlist_nulls_node to add a conntrack into two different lists.
As far as I understand, it's unacceptable in case of
SLAB_DESTROY_BY_RCU.
I guess you missed :

net/netfilter/nf_conntrack_core.c:1598: INIT_HLIST_NULLS_HEAD(&net->ct.unconfirmed, UNCONFIRMED_NULLS_VAL);

Lets imagine that we have two threads. The first one enumerates objects
from a first list, the second one recreates an object and insert it in a
second list.  If a first thread in this moment stays on the object, it
can read "next", when it's in the second list, so it will continue
to enumerate objects from the second list. It is not what we want, isn't
it?

cpu1				cpu2
				hlist_nulls_for_each_entry_rcu(ct)
destroy_conntrack
 kmem_cache_free

init_conntrack
 hlist_nulls_add_head_rcu
				ct = ct->next
This will be fine.

I think we even have a counter to count number of occurrence of this
rare event. (I personally never read a non null search_restart value)

 NF_CT_STAT_INC(net, search_restart); 
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help