[ofa-general] [PATCH] ipoib: racing uses of ipoib_neigh in CM

[akepner at sgi.com]

On Thu, Jun 18, 2009 at 07:54:22PM +0300, Yossi Etigin wrote:
> akepner at sgi.com wrote:
> > @@ -841,10 +841,20 @@ static void ipoib_set_mcast_list(struct net_device *dev)
> >  static void ipoib_neigh_cleanup(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;
> >  
> > +	/* 
> > +	 * Note that the read of the neigh pointer below is not protected 
> > +	 * by a ipoib_dev_priv->lock (since we don't yet know which device's 
> > +	 * lock to use). Count on the fact that if ipoib_neigh_free() has 
> > +	 * already freed the struct ipoib_neigh, to_ipoib_neigh() will 
> > +	 * return NULL.
> > +	 * 
> > +	 * If to_ipoib_neigh() does not return NULL, we'll re-read neigh 
> > +	 * under the appropriate lock below. 
> > +	 */
> >  	neigh = *to_ipoib_neigh(n);
> >  	if (neigh)
> >  		priv = netdev_priv(neigh->dev);
> 
> What if someone frees the neighbour right after you assign it to 'neigh'?
> 'neigh->dev' may become invalid, and so is the 'priv' and 'priv->spinlock'.
> 

There remains a window where that can happen, this patch just makes 
that window (quite a bit) smaller.

The best idea I've got for entirely preventing the race was suggested 
here:

http://lists.openfabrics.org/pipermail/general/2009-June/059841.html

(Seems like alot of complexity to add for this, though....)

Suggestions? 

-- 
Arthur