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

[Yossi Etigin]

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?

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?)
> 
>  - R.

-- 
--Yossi