Re: [PATCH] netfilter: ctnetlink: fix (really) race condition between dump_table and destroy

[Patrick McHardy <kaber@trash.net>]

Am 24.01.2011 15:06, schrieb Eric Dumazet:
> Le lundi 24 janvier 2011 à 14:37 +0100, Patrick McHardy a écrit :
>> On 24.01.2011 14:25, Pablo Neira Ayuso wrote:
>>> On 24/01/11 14:12, Eric Dumazet wrote:
>>>> Le lundi 24 janvier 2011 à 14:06 +0100, Pablo Neira Ayuso a écrit :
>>>>
>>>>> Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New
>>>>> patch attached.
>>>>
>>>> I feel now a bit uncomfortable, sorry ;)
>>>>
>>>> Are we sure the refcount cannot reach 0 while we hold
>>>> nf_conntrack_lock ?
>>>
>>> the ct deletion from the hash list is protected by spin lock, so
>>> whatever deletion would wait until we have left the dump section.
>>>
>>> with this patch, the code looks like it was in 2.6.24 before the rcu stuff.
>>
>> Yeah, we definitely have a reference while the conntrack is contained
>> in the hash table, and removal requires taking nf_conntrack_lock,
>> therefor using the conntrack entry while holding the lock is valid.
> 
> Yes, but to clarify my question, is the following possible ?
> 
> CPU 0                                           CPU1
> dump()
> spin_lock()
> ....
>     if (not enough room in skb)
> 						decrement last refcount
>       increment_ref_count()
> spin_unlock();
> 						since refcount hit 0,
> 						spin_lock(nf_conntrack)
> 						remove ct from hashes
> 

No, that can't happen. Before the last refcount is released, the entry
is removed from the hash, which requires to take the lock. Each entry
contained in the hash table has a refcount of at least 1.

If there are no futher concerns, I'm going to apply Pablo's patch.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html