[pve-devel] [PATCH pve-cluster] Fix #2553: Prevent the Deadlock by aligning the lockorder

[Kevin Greßlehner]

Kevin Greßlehner (1):
  Fix #2553: Fix for Deadlock by aligning the lockorder - Lock
    &memdb->mutex in memdb_read and refer to a new method
    "memdb_read_nolock" in memdb.c which doesn't handle locks by itself.
    This method then handles the stuff which was originally in
    memdb_read. Therefore everything except
    cfs_create_guest_conf_property_msg uses memdb_read (which handles
    the locking itself), and cfs_create_guest_conf_property_msg prelocks
    &memdb->mutex and invokes memdb_read_nolock.

Signed-off-by: Kevin Greßlehner <kevin_gressi at live.at>
---
Approach #3 (Comment #7 of Bug #2553):
Tested for 6h straight - no Crashes nor Deadlocks appeared.

 data/src/memdb.c  | 42 +++++++++++++++++++++++++++---------------
 data/src/memdb.h  |  6 ++++++
 data/src/status.c | 12 ++++++++----
 3 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/data/src/memdb.c b/data/src/memdb.c
index efc99ea..6c268df 100644
--- a/data/src/memdb.c
+++ b/data/src/memdb.c
@@ -659,32 +659,44 @@ int memdb_mkdir(
 	return ret;
 }
 
+//Original memdb_read without locking - Caller MUST handle the locking - Fix for #2553
+int
+memdb_read_nolock(
+        memdb_t *memdb,
+        const char *path,
+        gpointer *data_ret)
+{
+        g_return_val_if_fail(memdb != NULL, -EINVAL);
+        g_return_val_if_fail(path != NULL, -EINVAL);
+        g_return_val_if_fail(data_ret != NULL, -EINVAL);
+
+        memdb_tree_entry_t *te, *parent;
+
+        if ((te = memdb_lookup_path(memdb, path, &parent))) {
+                if (te->type == DT_REG) {
+                        *data_ret = g_memdup(te->data.value, te->size);
+                        guint32 size = te->size;
+                        return size;
+                }
+        }
+
+        return -ENOENT;
+}
+
 int 
 memdb_read(
 	memdb_t *memdb, 
 	const char *path, 
 	gpointer *data_ret)
 {
-	g_return_val_if_fail(memdb != NULL, -EINVAL);
-	g_return_val_if_fail(path != NULL, -EINVAL);
-	g_return_val_if_fail(data_ret != NULL, -EINVAL);
-
-	memdb_tree_entry_t *te, *parent; 
-
+	int res;
 	g_mutex_lock (&memdb->mutex);
 
-	if ((te = memdb_lookup_path(memdb, path, &parent))) {
-		if (te->type == DT_REG) {
-			*data_ret = g_memdup(te->data.value, te->size);
-			guint32 size = te->size;
-			g_mutex_unlock (&memdb->mutex);
-			return size;
-		}
-	}
+	res = memdb_read_nolock(memdb, path, data_ret);
 
 	g_mutex_unlock (&memdb->mutex);
 
-	return -ENOENT;
+	return res;
 }
 
 static int 
diff --git a/data/src/memdb.h b/data/src/memdb.h
index 60df035..7659358 100644
--- a/data/src/memdb.h
+++ b/data/src/memdb.h
@@ -162,6 +162,12 @@ memdb_read(
 	const char *path,
 	gpointer *data_ret);
 
+int
+memdb_read_nolock(
+        memdb_t *memdb,
+        const char *path,
+        gpointer *data_ret);
+
 int
 memdb_create(
 	memdb_t *memdb,
diff --git a/data/src/status.c b/data/src/status.c
index 491082c..1dd2737 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -883,6 +883,9 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 	int res = 0;
 	GString *path = NULL;
 
+	//Prelock &memdb->mutex in order to enable the usage of memdb_read_nolock
+	//to prevent Deadlocks as in #2553
+	g_mutex_lock (&memdb->mutex);
 	g_mutex_lock (&mutex);
 
 	g_string_printf(str,"{\n");
@@ -899,8 +902,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 		if (vminfo == NULL) goto enoent;
 
 		if (!vminfo_to_path(vminfo, path)) goto err;
-
-		int size = memdb_read(memdb, path->str, &tmp);
+		//use memdb_read_nolock because lock is handled here
+		int size = memdb_read_nolock(memdb, path->str, &tmp);
 		if (tmp == NULL) goto err;
 		if (size <= prop_len) goto ret;
 
@@ -924,7 +927,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
 
 			g_free(tmp); // no-op if already null
 			tmp = NULL;
-			int size = memdb_read(memdb, path->str, &tmp);
+			//use memdb_read_nolock because lock is handled here
+			int size = memdb_read_nolock(memdb, path->str, &tmp);
 			if (tmp == NULL || size <= prop_len) continue;
 
 			char *val = _get_property_value(tmp, size, prop, prop_len);
@@ -945,7 +949,7 @@ ret:
 	}
 	g_string_append_printf(str,"\n}\n");
 	g_mutex_unlock (&mutex);
-
+	g_mutex_unlock (&memdb->mutex);
 	return res;
 err:
 	res = -EIO;
-- 
2.20.1