Re: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path

[Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On Thu, Mar 04, 2010 at 10:58:27AM -0800, Ralph Campbell wrote:
> Subject: [PATCH v3] IB/ipoib: fix dangling pointer references to ipoib_neigh and ipoib_path
> 
> When using connected mode, ipoib_cm_create_tx() kmallocs a
> struct ipoib_cm_tx which contains pointers to ipoib_neigh and
> ipoib_path. If the paths are flushed or the struct neighbour is
> destroyed, the pointers held by struct ipoib_cm_tx can reference
> freed memory.
> 
I could use some more content here as this is quite a large patch.
Anyway below are some comments. I think besides reviewing this patch
we need to make sure it has been checked in real life.

> +/*
> + * Search the list of connections to be started and remove any entries
> + * which match the path being destroyed.
> + *
> + * This should be called with netif_tx_lock_bh() and priv->lock held.
> + */
> +void ipoib_cm_flush_path(struct net_device *dev, struct ipoib_path *path)
> +{
> +	struct ipoib_dev_priv *priv = netdev_priv(dev);
> +	struct ipoib_cm_tx *tx;
> +
> +	list_for_each_entry(tx, &priv->cm.start_list, list) {
> +		tx = list_entry(priv->cm.start_list.next, typeof(*tx), list);
> +		if (tx->path == path) {
> +			tx->path = NULL;
> +			list_del_init(&tx->list);
> +			break;
> +		}
>  	}
>  }

Looks to me like we may find struct ipoib_cm_tx objects hanging and
not freed. This could happen if they were just added to start_list and
removed by ipoib_cm_flush_path() before being processed by
ipoib_cm_tx_start().


> -static int __path_add(struct net_device *dev, struct ipoib_path *path)
> +static void __path_add(struct net_device *dev, struct ipoib_path *path)
>  {
>  	struct ipoib_dev_priv *priv = netdev_priv(dev);
>  	struct rb_node **n = &priv->path_tree.rb_node;
> @@ -249,44 +252,26 @@ static int __path_add(struct net_device *dev, struct ipoib_path *path)
>  			n = &pn->rb_left;
>  		else if (ret > 0)
>  			n = &pn->rb_right;
> -		else
> -			return -EEXIST;
> +		else /* Should never happen since we always search first */
> +			return;
>  	}

Why not remove the last else and change the "else if" into else?

>  	}
> @@ -460,19 +446,13 @@ static void path_rec_completion(int status,
>  			memcpy(&neigh->dgid.raw, &path->pathrec.dgid.raw,
>  			       sizeof(union ib_gid));
>  
> -			if (ipoib_cm_enabled(dev, neigh->neighbour)) {
> -				if (!ipoib_cm_get(neigh))
> -					ipoib_cm_set(neigh, ipoib_cm_create_tx(dev,
> -									       path,
> -									       neigh));
> -				if (!ipoib_cm_get(neigh)) {
> -					list_del(&neigh->list);
> -					if (neigh->ah)
> -						ipoib_put_ah(neigh->ah);
> -					ipoib_neigh_free(dev, neigh);
> -					continue;
> -				}
> -			}
ipoib_cm_set() is called only once in neigh_alloc(), setting neigh->cm
to  NULL, and never again so all calls to ipoib_cm_get() will return
NULL.

> +			/*
> +			 * If connected mode is enabled but not started,
> +			 * start getting a connection.
> +			 */
> +			if (ipoib_cm_enabled(dev, neigh->neighbour) &&
> +			    !ipoib_cm_get(neigh))
> +				ipoib_cm_create_tx(dev, path, neigh);
>  
>  			while ((skb = __skb_dequeue(&neigh->queue)))
>  				__skb_queue_tail(&skqueue, skb);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html