[ofw] [PATCH] librdmacm: fix race in ucma_init()

[Sean Hefty]

There's a potential race with ucma_init() and calls that check whether
the library is ready for use.  For example, in rdma_create_id, this
check is performed:

	hr = cma_dev_cnt ? 0 : ucma_init();
	if (hr) {
		return hr;
	}

Since the check for cma_dev_cnt is outside of any synchronization, if it
is non-zeroy, then the code will assume that initialization is complete.
However, ucma_init() can set cma_dev_cnt to non-zeroy before it has finished
executing.  The full impact of this race is unknown, but it can't be good.
In the worst case, the application may crash.

Fix this by ensuring that all initialization is complete before cma_dev_cnt
is incremented.  Note that ucma_init() provides synchronization to protect
against duplicate threads handling init.  The cma_dev_cnt check is used to
avoid synchronization on multiple calls into the library after initialization.

Signed-off-by: Sean Hefty <sean.hefty at intel.com>
---
This fix should go into winof 2.1.

Index: ulp/librdmacm/src/cma.cpp
===================================================================
--- ulp/librdmacm/src/cma.cpp	(revision 2373)
+++ ulp/librdmacm/src/cma.cpp	(working copy)
@@ -91,63 +91,49 @@
 static struct cma_device *cma_dev_array;
 static int cma_dev_cnt;
 
-static void ucma_cleanup(void)
-{
-	if (cma_dev_cnt > 0) {
-		while (cma_dev_cnt > 0) {
-			ibv_close_device(cma_dev_array[--cma_dev_cnt].verbs);
-		}
-		delete cma_dev_array;
-		cma_dev_cnt = 0;
-	}
-	if (windata.prov != NULL) {
-		ibvw_release_windata(&windata, IBVW_WINDATA_VERSION);
-		windata.prov = NULL;
-	}
-}
-
 static int ucma_init(void)
 {
 	struct ibv_device **dev_list = NULL;
 	struct cma_device *cma_dev;
 	struct ibv_device_attr attr;
-	int i, ret;
+	int i, ret, dev_cnt;
 
 	EnterCriticalSection(&lock);
-	if (cma_dev_cnt > 0) {
+	if (cma_dev_cnt) {
 		goto out;
 	}
 
 	ret = ibvw_get_windata(&windata, IBVW_WINDATA_VERSION);
 	if (ret) {
-		goto err;
+		goto err1;
 	}
 
-	dev_list = ibv_get_device_list(&cma_dev_cnt);
+	dev_list = ibv_get_device_list(&dev_cnt);
 	if (dev_list == NULL) {
 		ret = -1;
-		goto err;
+		goto err2;
 	}
 
-	cma_dev_array = new struct cma_device[cma_dev_cnt];
+	cma_dev_array = new struct cma_device[dev_cnt];
 	if (cma_dev_array == NULL) {
 		ret = -1;
-		goto err;
+		goto err3;
 	}
 
-	for (i = 0; dev_list[i]; ++i) {
+	for (i = 0; dev_list[i];) {
 		cma_dev = &cma_dev_array[i];
 
 		cma_dev->guid = ibv_get_device_guid(dev_list[i]);
 		cma_dev->verbs = ibv_open_device(dev_list[i]);
 		if (cma_dev->verbs == NULL) {
 			ret = -1;
-			goto err;
+			goto err4;
 		}
 
+		++i;
 		ret = ibv_query_device(cma_dev->verbs, &attr);
 		if (ret) {
-			goto err;
+			goto err4;
 		}
 
 		cma_dev->port_cnt = attr.phys_port_cnt;
@@ -155,16 +141,22 @@
 		cma_dev->max_responder_resources = (uint8_t)
attr.max_qp_rd_atom;
 	}
 	ibv_free_device_list(dev_list);
+	cma_dev_cnt = dev_cnt;
 out:
 	LeaveCriticalSection(&lock);
 	return 0;
 
-err:
-	ucma_cleanup();
-	LeaveCriticalSection(&lock);
-	if (dev_list) {
-		ibv_free_device_list(dev_list);
+err4:
+	while (i) {
+		ibv_close_device(cma_dev_array[--i].verbs);
 	}
+	delete cma_dev_array;
+err3:
+	ibv_free_device_list(dev_list);
+err2:
+	ibvw_release_windata(&windata, IBVW_WINDATA_VERSION);
+err1:
+	LeaveCriticalSection(&lock);
 	return ret;
 }
 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: librdmacm.diff
Type: application/octet-stream
Size: 2322 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20090824/2d6f20d0/attachment.obj>