Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Ying Xue <ying.xue@windriver.com>]

On 05/15/2015 11:39 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 15 May 2015 05:12:42 -0700
> 
>> On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote:
>>> Once modifying a pending timer of a neighbour, it's insufficient to
>>> post a warning message. Instead we should not take the neighbour's
>>> reference count at the same time, otherwise, it causes an issue that
>>> the neighbour cannot be freed forever.
>>>
>>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>>> ---
>>>  net/core/neighbour.c |    9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>> index 3de6542..5595db3 100644
>>> --- a/net/core/neighbour.c
>>> +++ b/net/core/neighbour.c
>>> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>>>  
>>>  static void neigh_add_timer(struct neighbour *n, unsigned long when)
>>>  {
>>> -	neigh_hold(n);
>>> -	if (unlikely(mod_timer(&n->timer, when))) {
>>> -		printk("NEIGH: BUG, double timer add, state is %x\n",
>>> -		       n->nud_state);
>>> +	if (likely(!mod_timer(&n->timer, when))) {
>>> +		neigh_hold(n);
>>> +	} else {
>>> +		pr_warn("NEIGH: BUG, double timer add, state is %x\n",
>>> +			n->nud_state);
>>>  		dump_stack();
>>>  	}
>>>  }
>>
>>
>> NACK
> 
> Indeed, major NACK.  And you've been told this change is unacceptable
> multiple times already, and you've been told exactly why as well.
> 

Eric, according to your below comments for the single patch,

http://www.spinics.net/lists/netdev/msg328828.html

I supposed the reason why you rejected it was because the issue of "neigh
use-after-free" was not resolved yet. More importantly, you did not explicitly
explain why the patch was wrong although I stated the problem the patch tries to
fix is different with the one discussed in the thread:

http://www.spinics.net/lists/netdev/msg323883.html

So, regarding my understanding for your comment, the patch can be acceptable if
the issue of "neigh use-after-free" is solved.

Since then I started to investigate its root cause and created the first patch
attempting to fix it. This is why I involve the patch into the series again.

If the issue of "neigh use-after-free" is fixed by the first patch although you
said the atomic_read() was not safe for us, is the patch still wrong? If it's
really wrong, can you please give more detailed explanation to help me
understand why the change is wrong and but why a similar timer usage in
sk_reset_timer() is not wrong?

Thanks,
Ying