[ofa-general] Re: [PATCH v3] ipoib: do not join broadcast group if interface is brought down

[Roland Dreier]

 > Roland Dreier wrote:
 > > So what protects priv->broadcast?  It seems that the only lock taken
 > > when setting broadcast to NULL is priv->lock.  But eg here:
 > >
 > > -	priv->mcast_mtu = IPOIB_UD_MTU(ib_mtu_enum_to_int(priv->broadcast->mcmember.mtu));
 > > +	if (priv->broadcast)
 > > +		priv->mcast_mtu = IPOIB_UD_MTU(ib_mtu_enum_to_int(priv->broadcast->mcmember.mtu));
 > >
 > > what prevents broadcast from becoming NULL right after it's tested?
 > >
 > > Also
 > >
 > > +	spin_lock_irq(&priv->lock);
 > > +	if (priv->broadcast &&
 > > +	    !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);
 > >
 > > doesn't ipoib_mcast_join() do GFP_KERNEL stuff, which would be a problem
 > > inside a spinlock?  (Have you tested this with lockdep turned on?)

 > Actually, priv->broadcast was never checked for being non-NULL
 > in mcast_join_task before this patch.
 > The only change in the patch is that priv->broadcast might stay NULL
 > if it was NULL when the function started. So, if anyone could set
 > priv->broadcast to NULL while join_task is running, wouldn't it happen
 > without the patch as well?

OK, so the race exists in the current code, but no one has hit it enough
to track it down.  Maybe we can fix it later.

But you ignored my second question about lock nesting problems?

 - R.