[ofa-general] Re: [RFC] [PATCH v3] IB/ipoib: Add bonding support to IPoIB

[Moni Shoua]

Michael S. Tsirkin wrote:
> Another question.
> 
>> @@ -769,32 +768,32 @@ static void ipoib_set_mcast_list(struct 
>>  static void ipoib_neigh_destructor(struct neighbour *n)
>>  {
>>  	struct ipoib_neigh *neigh;
>> -	struct ipoib_dev_priv *priv = netdev_priv(n->dev);
>> +	struct ipoib_dev_priv *priv;
>>  	unsigned long flags;
>>  	struct ipoib_ah *ah = NULL;
>>  
>> -	ipoib_dbg(priv,
>> -		  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
>> -		  IPOIB_QPN(n->ha),
>> -		  IPOIB_GID_RAW_ARG(n->ha + 4));
>> -
>> -	spin_lock_irqsave(&priv->lock, flags);
>>  
>>  	neigh = *to_ipoib_neigh(n);
>>  	if (neigh) {
>> +		priv = netdev_priv(neigh->dev);
>> +		ipoib_dbg(priv,
>> +			  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
>> +			  IPOIB_QPN(n->ha),
>> +			  IPOIB_GID_RAW_ARG(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);
>> +		spin_unlock_irqrestore(&priv->lock, flags);
>>  	}
>> -
>> -	spin_unlock_irqrestore(&priv->lock, flags);
>> -
>>  	if (ah)
>>  		ipoib_put_ah(ah);
>>  }
> 
> Using to_ipoib_neigh outside priv->lock looks problematic.
> Can you convince me this does not introduce new races?
> 
> 
I can try...
ipoib_neigh_destructor is called from neigh_destroy() and this is when the kernel neighbour 
is under destruction itself and no one holds a reference to it. 
My opinion is that if I can't assume that no one is touching ipoib_neigh when kernel 
neighbour is being destroyed then we have a bigger problem.