From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 7 Nov 2024 10:08:40 +0000 (+0100)
Subject: BUG/MINOR: guid/server: ensure thread-safety on GUID insert/delete
X-Git-Tag: v3.1-dev12~4
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8e0e7d9d1af5b2dfec2e625d2c19dd034c36eb04;p=thirdparty%2Fhaproxy.git

BUG/MINOR: guid/server: ensure thread-safety on GUID insert/delete

Since 3.0, it is possible to assign a GUID to proxies, listeners and
servers. These objects are stored in a global tree guid_tree.

Proxies and listeners are static. However, servers may be added or
deleted at runtime, which imply that guid_tree must be protected. Fix
this by declaring a read-write lock to protect tree access.

For now, only guid_insert() and guid_remove() are protected using a
write lock. Outside of these, GUID tree is not accessed at runtime. If
server CLI commands are extended to support GUID as server identifier,
lookup operation should be extended with a read lock protection.

Note that during stat-file preloading, GUID tree is accessed for lookup.
However, as it is performed on startup which is single threaded, there
is no need for lock here. A BUG_ON() has been added to ensure this
precondition remains true.

This bug could caused a segfault when using dynamic servers with GUID.
However, it was never reproduced for now.

This must be backported up to 3.0. To avoid a conflict issue, the
previous cleanup patch can be merged before it.
---

diff --git a/include/haproxy/guid.h b/include/haproxy/guid.h
index a605c8668b..691936ffe3 100644
--- a/include/haproxy/guid.h
+++ b/include/haproxy/guid.h
@@ -1,7 +1,11 @@
 #ifndef _HAPROXY_GUID_H
 #define _HAPROXY_GUID_H
 
+#include <haproxy/api-t.h>
 #include <haproxy/guid-t.h>
+#include <haproxy/thread-t.h>
+
+__decl_thread(extern HA_RWLOCK_T guid_lock);
 
 void guid_init(struct guid_node *node);
 int guid_insert(enum obj_type *obj_type, const char *uid, char **errmsg);
diff --git a/include/haproxy/thread-t.h b/include/haproxy/thread-t.h
index 5d36107393..dd2fc6260b 100644
--- a/include/haproxy/thread-t.h
+++ b/include/haproxy/thread-t.h
@@ -205,6 +205,7 @@ enum lock_label {
 	OCSP_LOCK,
 	QC_CID_LOCK,
 	CACHE_LOCK,
+	GUID_LOCK,
 	OTHER_LOCK,
 	/* WT: make sure never to use these ones outside of development,
 	 * we need them for lock profiling!
diff --git a/src/guid.c b/src/guid.c
index f1365b621c..567669cd7b 100644
--- a/src/guid.c
+++ b/src/guid.c
@@ -6,9 +6,11 @@
 #include <haproxy/proxy.h>
 #include <haproxy/server-t.h>
 #include <haproxy/tools.h>
+#include <haproxy/thread.h>
 
 /* GUID global tree */
 struct eb_root guid_tree = EB_ROOT_UNIQUE;
+__decl_thread(HA_RWLOCK_T guid_lock);
 
 /* Initialize <guid> members. */
 void guid_init(struct guid_node *guid)
@@ -60,13 +62,17 @@ int guid_insert(enum obj_type *objt, const char *uid, char **errmsg)
 	}
 
 	guid->node.key = key;
+
+	HA_RWLOCK_WRLOCK(GUID_LOCK, &guid_lock);
 	node = ebis_insert(&guid_tree, &guid->node);
 	if (node != &guid->node) {
 		dup = ebpt_entry(node, struct guid_node, node);
+		HA_RWLOCK_WRUNLOCK(GUID_LOCK, &guid_lock);
 		dup_name = guid_name(dup);
 		memprintf(errmsg, "duplicate entry with %s", dup_name);
 		goto err;
 	}
+	HA_RWLOCK_WRUNLOCK(GUID_LOCK, &guid_lock);
 
 	guid->obj_type = objt;
 	return 0;
@@ -82,8 +88,10 @@ int guid_insert(enum obj_type *objt, const char *uid, char **errmsg)
  */
 void guid_remove(struct guid_node *guid)
 {
+	HA_RWLOCK_WRLOCK(GUID_LOCK, &guid_lock);
 	ebpt_delete(&guid->node);
 	ha_free(&guid->node.key);
+	HA_RWLOCK_WRUNLOCK(GUID_LOCK, &guid_lock);
 }
 
 /* Retrieve an instance from GUID global tree with key <uid>.
@@ -95,6 +103,12 @@ struct guid_node *guid_lookup(const char *uid)
 	struct ebpt_node *node = NULL;
 	struct guid_node *guid = NULL;
 
+	/* For now, guid_lookup() is only used during startup in single-thread
+	 * mode. If this is not the case anymore, GUID tree access must be
+	 * protected with the read-write lock.
+	 */
+	BUG_ON(!(global.mode & MODE_STARTING));
+
 	node = ebis_lookup(&guid_tree, uid);
 	if (node)
 		guid = ebpt_entry(node, struct guid_node, node);
diff --git a/src/thread.c b/src/thread.c
index 655e1998cf..810420a789 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -462,6 +462,7 @@ static const char *lock_label(enum lock_label label)
 	case OCSP_LOCK:            return "OCSP";
 	case QC_CID_LOCK:          return "QC_CID";
 	case CACHE_LOCK:           return "CACHE";
+	case GUID_LOCK:            return "GUID";
 	case OTHER_LOCK:           return "OTHER";
 	case DEBUG1_LOCK:          return "DEBUG1";
 	case DEBUG2_LOCK:          return "DEBUG2";