Re: [PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>> In patch 5 you refer to timer activation and hash insertion. Why does
>> helper lookup needs to be done inside the lock?
> 
> 
> On helper module removal, the conntracks in the hashes are unhelped but
> the conntrack that we are about to create is not yet in hashes, and it
> will be refering to a unexistant helper. To overcome this Harald added
> the function helper_find_get() that uses appropiate module refcounting,
> so the helper can not vanish in the ctnetlink conntrack creation path.
> At the time that I was cooking the patch, I realised that we don't need
> find_get() anymore if we can move the helper lookup inside the locked
> section.

Yes, that seems right.

>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>>
>>> Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
>>> ===================================================================
>>> --- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c   
>>> 2006-07-07 00:15:14.000000000 +0200
>>> +++ net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c    2006-07-07
>>> 01:52:14.000000000 +0200
>>> @@ -1059,13 +1059,12 @@ ctnetlink_create_conntrack(struct nfattr
>>>         ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));
>>> #endif
>>>
>>> -    ct->helper = ip_conntrack_helper_find_get(rtuple);
>>> -
>>> -    add_timer(&ct->timeout);
>>> +    /* we do no want any races on hash insertion */
>>> +    write_lock_bh(&ip_conntrack_lock);
>>> +    ct->helper = ip_conntrack_helper_find(rtuple);
>>>     ip_conntrack_hash_insert(ct);
>>> -
>>> -    if (ct->helper)
>>> -        ip_conntrack_helper_put(ct->helper);
>>> +    add_timer(&ct->timeout);
>>> +    write_unlock_bh(&ip_conntrack_lock);
>>>
>>>     DEBUGP("conntrack with id %u inserted\n", ct->id);
>>>     return 0;
>>
>>
>>
>> I also see another race, the caller of create_conntrack does a lookup
>> within locks, drops the locks and we reaquire them here. The entire
>> lookup-insertion needs to be atomic.
> 
> 
> Not really, the lookup-update must be atomic, as it is now, since the
> conntrack is in the hashes, but for the lookup-creation-insertion I
> don't see why we would need to. The conntrack that we are creating is
> not yet in the hashes, therefore we only need the locking in the step of
> timer activation and insertion.

The insertion doesn't check for existance again, but it might
have already been created asynchrously between the lookup and
the insertion, in which case we end up with two identical
conntracks in the hash table.