[openib-general] Re: [PATCH] cm refcount race fix

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier at cisco.com>:
> But I think changing atomic_t to an integer protected by a lock it
> much cleaner anyway.

We'll have to change each and every access to the locked version then though.
It would be a big risky change. How about the following compromize?

----

Fix race condition in CM.
use after free if ib_destroy_cm_id tests the refcount after cm_deref_id has
decremented the reference count but before it has called wake_up.

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

Index: linux-2.6.16/drivers/infiniband/core/cm.c
===================================================================
--- linux-2.6.16.orig/drivers/infiniband/core/cm.c	2006-05-08 19:16:10.000000000 +0300
+++ linux-2.6.16/drivers/infiniband/core/cm.c	2006-05-08 19:19:05.000000000 +0300
@@ -159,8 +159,12 @@ static void cm_work_handler(void *data);
 
 static inline void cm_deref_id(struct cm_id_private *cm_id_priv)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&cm_id_priv->lock, flags);
 	if (atomic_dec_and_test(&cm_id_priv->refcount))
 		wake_up(&cm_id_priv->wait);
+	spin_unlock_irqrestore(&cm_id_priv->lock, flags);
 }
 
 static int cm_alloc_msg(struct cm_id_private *cm_id_priv,
@@ -710,6 +714,16 @@ static void cm_reset_to_idle(struct cm_i
 	}
 }
 
+static int cm_get_count(struct cm_id_private *cm_id_priv)
+{
+	int count;
+	/* Lock makes sure cm_deref_id is not in progress */
+	spin_lock_irq(&cm_id_priv->lock);
+	count = atomic_read(&cm_id_priv->refcount)
+	spin_unlock_irq(&cm_id_priv->lock);
+	return count;
+}
+
 void ib_destroy_cm_id(struct ib_cm_id *cm_id)
 {
 	struct cm_id_private *cm_id_priv;
@@ -777,7 +791,8 @@ retest:
 
 	cm_free_id(cm_id->local_id);
 	atomic_dec(&cm_id_priv->refcount);
-	wait_event(cm_id_priv->wait, !atomic_read(&cm_id_priv->refcount));
+	wait_event(cm_id_priv->wait, !cm_get_count(cm_id_priv));
+
 	while ((work = cm_dequeue_work(cm_id_priv)) != NULL)
 		cm_free_work(work);
 	kfree(cm_id_priv->compare_data);

-- 
MST