Re: [PATCH net] net: neighbour: add neighbour dead check for neigh_timer_handler()

[Gao feng <gaofeng@cn.fujitsu.com>]

On 12/04/2013 05:16 PM, Ding Tianhong wrote:
> On 2013/12/4 14:27, Eric Dumazet wrote:
>> On Wed, 2013-12-04 at 14:19 +0800, Ding Tianhong wrote:
>>> On 2013/12/4 12:21, David Miller wrote:
>>>> From: Ding Tianhong <dingtianhong@huawei.com>
>>>> Date: Wed, 4 Dec 2013 12:04:31 +0800
>>>>
>>>>> The destroying neigh could be trigger by userspace, just like set the ip address which
>>>>> in arp table to the local device ip, some I could not control it, it maybe anytime,
>>>>> but the timer handler is execute by logic, this is normal, so I think the logic
>>>>> is no problem, and the process of destroying neigh may conflict with the timer handler,
>>>>> it is a synchronous problem to make sure the timer should be finished before the
>>>>> reference neigh is freed.
>>>>
>>>> The more I think about this, the more none of the explanations for this bug
>>>> make any sense.
>>>>
>>>> neigh_destroy() _ONLY_ runs when:
>>>>
>>>> 	if (atomic_dec_and_test(&neigh->refcnt))
>>>>
>>>> triggers in neigh_release().
>>>>
>>>> This means it triggers if, and only if, neigh_refcnt goes to zero.
>>>>
>>>> If the refcnt goes to zero, NO TIMER can be running.  If the timer is
>>>> running, then there refcnt must be at least '1'.
>>>
>>> Hi David:
>>>
>>> Yes, you are right, but when the timer is running and prior to get the neigh->lock, the refcnt
>>> could be dec to 0, you could not stop it by existing mechanism.
>>>
>>> the refcnt of neighbour could only be inc by these actions:
>>>
>>> 1.create neighbour, the refcnt will be set to 1.
>>> 2.add timer, the refcnt++.
>>> 3.neigh_lookup, if found the neigh, refcnt++.
>>>
>>> I can show the whole process of my analysis:
>>>
>>> 		CPU 0				CPU 1
>>> 		-----				-----
>>> 	create_neigh() => refcnt = 1;		
>>> 	add timer =>	refcnt++;
>>> 						<SOFTIRQ>
>>> 						base->running_timer = neigh->timer;
>>> 						neigh_timer_handler() => at this time, refcnt is 2;
>>>
>>> user->	neigh_changeaddr()
>>> 	neigh_flush_dev();
>>> 	neigh_del_imer, refcnt dec to 1;
>>
>> Nope : del_timer() would return 0 here, so we do not decrement refcnt.
>>
> 
> The first call for del_timer() will return 1, because the timer->entry.next is not NULL,
> then in the neigh_destroy, the del_timer() again will return 0 because timer->entry.next is NULL. 
> 
>> I can tell you, if this was not the case, a lot of things would be
>> terribly broken, like TCP stack.
>>
>>> 	release_neigh(), refcnt is 0,
>>> 	destroy_neigh()
>>> 	kfree(neighbour);
>>> 						write(neigh->lock)
>>>
>>> So in my opinion, the point of the problem is that I should not kfree the neighbour until
>>> the timer is not running on CPUs and not pending.
>>>
>>> If I miss someghing, pls point out.
>>
>> As David explained, if a timer is running, refcnt can not reach 0,
>> untill the timer handler finished.
>>
>> So _something_ is calling neigh_release(n) without prior neigh_hold()
>>

seems like neigh_release in neigh_del_timer should be removed, it can't
release the neigh for the timer which may want to call neigh_release too,
this release job should be done by timer handler.

we should make sure if the timer is not pending, it doesn't have the
reference of neigh.

It's great if we can reproduce this bug, but...