[ofa-general] [PATCH RFC] IB/ipoib: fix to_ipoib_neigh access race

[Michael S. Tsirkin]

hard_start_xmit dereferences to_ipoib_neigh when only tx_lock is taken.  This
would only be safe if all calls that modify *to_ipoib_neigh take tx_lock too.
Currently this is not always true for ipoib_neigh_free and path_rec_completion,
which results in memory corruption.  Fix this race, making sure
path_rec_completion and ipoib_neigh_free are always called under
tx_lock.

Signed-off-by: Michael S. Tsirkin <mst at dev.mellanox.co.il>

---

I'm looking at
https://bugs.openfabrics.org/show_bug.cgi?id=604
and I think this could explain the crashes.
In any case, Roland, is there a race or am I imagining things?

NB: The patch is untested (I'm not at the lab now).

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 0a428f2..ef9845a 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -364,9 +364,9 @@ void ipoib_flush_paths(struct net_device *dev)
 		spin_unlock(&priv->lock);
 		spin_unlock_irq(&priv->tx_lock);
 		wait_for_completion(&path->done);
-		path_free(dev, path);
 		spin_lock_irq(&priv->tx_lock);
 		spin_lock(&priv->lock);
+		path_free(dev, path);
 	}
 	spin_unlock(&priv->lock);
 	spin_unlock_irq(&priv->tx_lock);
@@ -401,7 +401,8 @@ static void path_rec_completion(int status,
 			ah = ipoib_create_ah(dev, priv->pd, &av);
 	}
 
-	spin_lock_irqsave(&priv->lock, flags);
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	spin_lock(&priv->lock);
 
 	path->ah = ah;
 
@@ -442,7 +443,8 @@ static void path_rec_completion(int status,
 	path->query = NULL;
 	complete(&path->done);
 
-	spin_unlock_irqrestore(&priv->lock, flags);
+	spin_unlock(&priv->lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	while ((skb = __skb_dequeue(&skqueue))) {
 		skb->dev = dev;
@@ -822,7 +824,8 @@ static void ipoib_neigh_cleanup(struct neighbour *n)
 		  IPOIB_QPN(n->ha),
 		  IPOIB_GID_RAW_ARG(n->ha + 4));
 
-	spin_lock_irqsave(&priv->lock, flags);
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	spin_lock(&priv->lock);
 
 	neigh = *to_ipoib_neigh(n);
 	if (neigh) {
@@ -832,7 +835,8 @@ static void ipoib_neigh_cleanup(struct neighbour *n)
 		ipoib_neigh_free(n->dev, neigh);
 	}
 
-	spin_unlock_irqrestore(&priv->lock, flags);
+	spin_unlock(&priv->lock, flags);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	if (ah)
 		ipoib_put_ah(ah);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 54fbead..d2e6a1a 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -100,7 +100,8 @@ static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 			"deleting multicast group " IPOIB_GID_FMT "\n",
 			IPOIB_GID_ARG(mcast->mcmember.mgid));
 
-	spin_lock_irqsave(&priv->lock, flags);
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	spin_lock(&priv->lock);
 
 	list_for_each_entry_safe(neigh, tmp, &mcast->neigh_list, list) {
 		/*
@@ -114,7 +115,8 @@ static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 		ipoib_neigh_free(dev, neigh);
 	}
 
-	spin_unlock_irqrestore(&priv->lock, flags);
+	spin_unlock(&priv->lock);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	if (mcast->ah)
 		ipoib_put_ah(mcast->ah);

-- 
MST