[ofa-general] Re: IPoIB kernel Oops -- possible race condition identified.

[Jack Morgenstein]

On Wednesday 28 January 2009 20:53, Roland Dreier wrote:
>  > -	priv->mcast_mtu = IPOIB_UD_MTU(ib_mtu_enum_to_int(priv->broadcast->mcmember.mtu));
>  > +	spin_lock_irq(&priv->lock);
>  > +	if (priv->broadcast)
>  > +		priv->mcast_mtu = IPOIB_UD_MTU(ib_mtu_enum_to_int(priv->broadcast->mcmember.mtu));
>  > +	else
>  > +		priv->mcast_mtu = priv->admin_mtu;
> 
> Looks reasonable, except what about the code shortly before that in
> ipoib_mcast_join_task():
> 
> 	if (!test_bit(IPOIB_MCAST_FLAG_ATTACHED, &priv->broadcast->flags)) {
> 		if (!test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags))
> 			ipoib_mcast_join(dev, priv->broadcast, 0);
> 		return;
> 	}
> 
> is there any reason why that is safe without locking around the using
> priv->broadcast?  (Fixing that place looks harder, since
> ipoib_mcast_join() currently may sleep)
> 
You're right! (I just did not notice this -- was focussed on our Oops).
I also do not see any reasonable solution as yet -- as soon as we release
the spinlock, priv->broadcast may become NULL if flush is going on.
Furthermore, even if we copied the pointer, passing priv->broadcast as a parameter
in ipoib_mcast_join is also problematic, since the pointer may become obsolete
while ipoib_mcast_join is running. 

Seems to me that we may need a reference counter on priv->broadcast so that it does
not get freed to soon (in ipoib_mcast_free).  This means that all multicast objects need
reference counting.

Ouch!