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

[Patrick McHardy <kaber@trash.net>]

On 24.01.2011 12:57, Pablo Neira Ayuso wrote:
> On 24/01/11 07:41, Eric Dumazet wrote:
>> Le lundi 24 janvier 2011 à 00:16 +0100, Pablo Neira Ayuso a écrit :
>>> In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks
>>> to protect the dump of the conntrack table according to reports from
>>> Stephen and acknowledgments on the issue from Eric.
>>
>> When a commit is referred, always give its title, and you can use a
>> short id (12 digits are OK)
> 
> I'm using the id that git log --oneline shows, is it OK?
> 
>> In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and
>> destroy)
>>
>>> However, Stephen removed the refcount bump in that patch that allows
>>> to keep a reference to the current ct object we are interating over.
>>> That code avoids race conditions between ct object destruction and
>>> the iteration itself. This patch reintroduces these lines since the
>>> ct object may vanish between two recvmgs() invocations.
>>>
>>> This patch fixes ocasional crashes while dumping the conntrack table
>>> intensively.
>>>
>>> Cc: Stephen Hemminger <stephen.hemminger@vyatta.com>
>>
>> Thats not true, Stephen was not in CC on your mail
> 
> I'm sorry, I forgot to add --cc to stgit.
> 
>> I add him right now.
>>
>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>> ---
>>>  net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
>>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>>> index 93297aa..519e245 100644
>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>> @@ -654,14 +654,16 @@ restart:
>>>  			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
>>>  				continue;
>>>  			ct = nf_ct_tuplehash_to_ctrack(h);
>>> +			if (!atomic_inc_not_zero(&ct->ct_general.use))
>>> +				continue;
>>>  			/* Dump entries of a given L3 protocol number.
>>>  			 * If it is not specified, ie. l3proto == 0,
>>>  			 * then dump everything. */
>>>  			if (l3proto && nf_ct_l3num(ct) != l3proto)
>>> -				continue;
>>> +				goto releasect;
>>>  			if (cb->args[1]) {
>>>  				if (ct != last)
>>> -					continue;
>>> +					goto releasect;
>>>  				cb->args[1] = 0;
>>>  			}
>>>  			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
>>> @@ -679,6 +681,8 @@ restart:
>>>  				if (acct)
>>>  					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
>>>  			}
>>> +releasect:
>>> +		nf_ct_put(ct);
>>>  		}
>>>  		if (cb->args[1]) {
>>>  			cb->args[1] = 0;
>>>
>>
>>
>> Hmm...
>>
>> Only the RCU lookup should need this extra check (atomic_inc_not_zero),
>> not a writer. (And a dumper is a "writer" since it holds nf_conntrack
>> spinlock)
>>
>> In Stephen commit, we switched from RCU lookup to traditional one
>> (spinlock protected), so, we should not need to touch individual objects
>> refcount ?
>>
>> I feel the right fix is not to increment refcount then decrement it.
>>
>> Only to _test_ it being zero, and not dumping the ct, eventually ?
>>
>>
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index 93297aa..a977cc7 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -654,6 +654,8 @@ restart:
>>  			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
>>  				continue;
>>  			ct = nf_ct_tuplehash_to_ctrack(h);
>> +			if (!atomic_read(&ct->ct_general.use))
>> +				continue;
>>  			/* Dump entries of a given L3 protocol number.
>>  			 * If it is not specified, ie. l3proto == 0,
>>  			 * then dump everything. */
> 
> No, that won't work.
> 
> Sorry, I didn't explain the problem appropriately. We still do a
> nf_ct_put() for last ct. In that patch, you forgot to add the refcount
> bump that we need to keep the reference to the object.
> 
> The following patch attached is smaller, it also fixes the problem and
> we don't have to increase the refcount for each object. Now it looks
> similar to how it was before the RCU patches.

This looks correct to me, we need to keep a reference for continuations.
Eric, do you agree with this 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