[openib-general] [PATCH]IPOIB: adding spin_lock in path_free()

[Roland Dreier]

This patch worried me, because dropping the lock between
path_rec_start() failing and calling path_free() opens a window where
the path is in our path table with no locks held.  Also dropping the
lock halfway through path_free() made me nervous too.

I thought a little about all this and came up with the following
patch.  This moves the __path_add() out of path_rec_create() so that
it's OK to drop the lock in unicast_arp_send() before calling
path_free().  Also, it uses priv->lock to serialize the freeing of
IPoIB neighbour structs.

Do you see any problems with this?

Thanks,
  Roland

--- infiniband/ulp/ipoib/ipoib_main.c	(revision 1909)
+++ infiniband/ulp/ipoib/ipoib_main.c	(working copy)
@@ -215,16 +215,25 @@ static int __path_add(struct net_device 
 	return 0;
 }
 
-static void __path_free(struct net_device *dev, struct ipoib_path *path)
+static void path_free(struct net_device *dev, struct ipoib_path *path)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 	struct ipoib_neigh *neigh, *tn;
 	struct sk_buff *skb;
+	unsigned long flags;
 
 	while ((skb = __skb_dequeue(&path->queue)))
 		dev_kfree_skb_irq(skb);
 
+	spin_lock_irqsave(&priv->lock, flags);
+
 	list_for_each_entry_safe(neigh, tn, &path->neigh_list, list) {
+		/*
+		 * It's safe to call ipoib_put_ah() inside priv->lock
+		 * here, because we know that path->ah will always
+		 * hold one more reference, so ipoib_put_ah() will
+		 * never do more than decrement the ref count.
+		 */
 		if (neigh->ah)
 			ipoib_put_ah(neigh->ah);
 		*to_ipoib_neigh(neigh->neighbour) = NULL;
@@ -232,11 +241,11 @@ static void __path_free(struct net_devic
 		kfree(neigh);
 	}
 
+	spin_unlock_irqrestore(&priv->lock, flags);
+
 	if (path->ah)
 		ipoib_put_ah(path->ah);
 
-	rb_erase(&path->rb_node, &priv->path_tree);
-	list_del(&path->list);
 	kfree(path);
 }
 
@@ -248,15 +257,20 @@ void ipoib_flush_paths(struct net_device
 	unsigned long flags;
 
 	spin_lock_irqsave(&priv->lock, flags);
+
 	list_splice(&priv->path_list, &remove_list);
 	INIT_LIST_HEAD(&priv->path_list);
+
+	list_for_each_entry(path, &remove_list, list)
+		rb_erase(&path->rb_node, &priv->path_tree);
+
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	list_for_each_entry_safe(path, tp, &remove_list, list) {
 		if (path->query)
 			ib_sa_cancel_query(path->query_id, path->query);
 		wait_for_completion(&path->done);
-		__path_free(dev, path);
+		path_free(dev, path);
 	}
 }
 
@@ -361,8 +375,6 @@ static struct ipoib_path *path_rec_creat
 	path->pathrec.pkey      = cpu_to_be16(priv->pkey);
 	path->pathrec.numb_path = 1;
 
-	__path_add(dev, path);
-
 	return path;
 }
 
@@ -422,6 +434,8 @@ static void neigh_add_path(struct sk_buf
 				       (union ib_gid *) (skb->dst->neighbour->ha + 4));
 		if (!path)
 			goto err;
+
+		__path_add(dev, path);
 	}
 
 	list_add_tail(&neigh->list, &path->neigh_list);
@@ -497,8 +511,12 @@ static void unicast_arp_send(struct sk_b
 			skb_push(skb, sizeof *phdr);
 			__skb_queue_tail(&path->queue, skb);
 
-			if (path_rec_start(dev, path))
-				__path_free(dev, path);
+			if (path_rec_start(dev, path)) {
+				spin_unlock(&priv->lock);
+				path_free(dev, path);
+				return;
+			} else
+				__path_add(dev, path);
 		} else {
 			++priv->stats.tx_dropped;
 			dev_kfree_skb_any(skb);
@@ -658,7 +676,7 @@ static void ipoib_set_mcast_list(struct 
 
 static void ipoib_neigh_destructor(struct neighbour *n)
 {
-	struct ipoib_neigh *neigh = *to_ipoib_neigh(n);
+	struct ipoib_neigh *neigh;
 	struct ipoib_dev_priv *priv = netdev_priv(n->dev);
 	unsigned long flags;
 	struct ipoib_ah *ah = NULL;
@@ -670,6 +688,7 @@ static void ipoib_neigh_destructor(struc
 
 	spin_lock_irqsave(&priv->lock, flags);
 
+	neigh = *to_ipoib_neigh(n);
 	if (neigh) {
 		if (neigh->ah)
 			ah = neigh->ah;