[ofa-general] [RFC] ipoib: avoid using stale ipoib_neigh* in ipoib_neigh_cleanup()

[Yossi Etigin]

akepner at sgi.com wrote:
> On Fri, May 22, 2009 at 01:24:17PM +0300, Yossi Etigin wrote:
>> ...
>> So, ipoib tries to list_del(neigh) twice because the second time
>> the condition (neigh != NULL) is not protected with a lock.
>> How about this:
>>
>> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>> index ab2c192..993b5a7 100644
>> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
>> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
>> @@ -845,23 +845,24 @@ static void ipoib_neigh_cleanup(struct neighbour *n)
>>  	unsigned long flags;
>>  	struct ipoib_ah *ah = NULL;
>>  
>> +	spin_lock_irqsave(&priv->lock, flags); <-- deadlock here
>> +
>>  	neigh = *to_ipoib_neigh(n);
>>  	if (neigh)
>>  		priv = netdev_priv(neigh->dev);
>>  	else
>> -		return;
>> +		goto out;
>>  	ipoib_dbg(priv,
>>  		  "neigh_cleanup for %06x %pI6\n",
>>  		  IPOIB_QPN(n->ha),
>>  		  n->ha + 4);
>>  
>> -	spin_lock_irqsave(&priv->lock, flags);
>> -
>>  	if (neigh->ah)
>>  		ah = neigh->ah;
>>  	list_del(&neigh->list);
>>  	ipoib_neigh_free(n->dev, neigh);
>>  
>> +out:
>>  	spin_unlock_irqrestore(&priv->lock, flags);
>>  
>>  	if (ah)
>>
> 
> This is essentially what I did first time around, but a deadlock on 
> the line marked above was quickly found. 
> 

Interesting... what does it deadlock with?
And what is the hole your fix leaves? If the (neigh!=NULL) check passes
with the spinlock held, shouldn't it be OK to list_del() it?

--Yossi