xen-dom-mgmt: Add refcount to domain structure

Add reference counting to the domain structure to make accessing it safe without global locks. domain_create initializes the refcount to 1 to prevent the domain from being freed right away, this original reference is dropped in domain_destroy to trigger freeing the domain. Also update test headers to keep them in sync with the changes. Signed-off-by: Mykyta Poturai <[email protected]> Reviewed-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]>
xen-troops · Aug 5, 2024 · 801d3b1 · 801d3b1
1 parent e82c350
commit 801d3b1
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 57 deletions.
diff --git a/tests/xrun/include/domain.h b/tests/xrun/include/domain.h
@@ -117,6 +117,7 @@ struct xen_domain_console {
 struct xen_domain {
 };
 
-struct xen_domain *domid_to_domain(uint32_t domid);
+struct xen_domain *get_domain(uint32_t domid);
+void put_domain(struct xen_domain *domain);
 
 #endif /* XENLIB_XEN_DOMAIN_H */
diff --git a/xen-console-srv/src/xen_console.c b/xen-console-srv/src/xen_console.c
@@ -480,6 +480,7 @@ int xen_attach_domain_console(const struct shell *shell,
 		/* Actually, this should never happen */
 		shell_error(shell, "Shell is already attached to console");
 		k_mutex_unlock(&global_console_lock);
+		put_domain(domain);
 		return -EEXIST;
 	}
 	current_console = console;
@@ -500,6 +501,7 @@ int xen_attach_domain_console(const struct shell *shell,
 
 	shell_set_bypass(shell, console_shell_cb);
 
+	put_domain(domain);
 	return 0;
 }
 

diff --git a/xen-dom-mgmt/include/domain.h b/xen-dom-mgmt/include/domain.h
@@ -202,6 +202,7 @@ struct xen_domain {
 	int address_size;
 	uint64_t max_mem_kb;
 	sys_dnode_t node;
+	int refcount;
 
 	/* TODO: domains can have more than one console */
 	struct xen_domain_console console;
@@ -210,7 +211,8 @@ struct xen_domain {
 	bool f_dom0less:1; /**< Indicates that domain was created by Xen itself */
 };
 
-struct xen_domain *domid_to_domain(uint32_t domid);
+struct xen_domain *get_domain(uint32_t domid);
+void put_domain(struct xen_domain *domain);
 
 #ifdef __cplusplus
 }

diff --git a/xen-dom-mgmt/src/xen-dom-mgmt.c b/xen-dom-mgmt/src/xen-dom-mgmt.c
@@ -584,23 +584,64 @@ static int assign_dtdevs(int domid, char *dtdevs[], int nr_dtdevs)
 	return rc;
 }
 
-/*
- * TODO: Access to domain_list and domains should be protected, considering that it may be
- * destroyed after receiving pointer to actual domain. So all accesses to domains structs should be
- * protected globally or via refcounts. This requires code audit in all libs, that are using this
- * function (currently xenstore-srv and xen_shell).
- */
-struct xen_domain *domid_to_domain(uint32_t domid)
+struct xen_domain *get_domain(uint32_t domid)
 {
 	struct xen_domain *iter;
 
+	k_mutex_lock(&dl_mutex, K_FOREVER);
 	SYS_DLIST_FOR_EACH_CONTAINER (&domain_list, iter, node) {
 		if (iter->domid == domid) {
-			return iter;
+			iter->refcount++;
+			break;
 		}
 	}
+	k_mutex_unlock(&dl_mutex);
+	return iter;
+}
 
-	return NULL;
+void put_domain(struct xen_domain *domain)
+{
+	int rc;
+
+	if (!domain) {
+		LOG_ERR("Domain is NULL");
+		return;
+	}
+	__ASSERT(!domain->f_dom0less, "dom0less domain#%u operation not supported", domain->domid);
+
+	k_mutex_lock(&dl_mutex, K_FOREVER);
+	domain->refcount--;
+	if (domain->refcount == 0) {
+		rc = xs_remove_xenstore_backends(domain);
+		if (rc) {
+			LOG_ERR("Failed to remove_xenstore_backends domain#%u (rc=%d)",
+					domain->domid, rc);
+		}
+
+		rc = stop_domain_stored(domain);
+		if (rc) {
+			LOG_ERR("Failed to stop domain#%u store (rc=%d)", domain->domid, rc);
+		}
+
+		xs_deinitialize_domain_xenstore(domain->domid);
+
+	#ifdef CONFIG_XEN_CONSOLE_SRV
+		rc = xen_stop_domain_console(domain);
+		if (rc) {
+			LOG_ERR("Failed to stop domain#%u console (rc=%d)", domain->domid, rc);
+		}
+	#endif
+
+		rc = xen_domctl_destroydomain(domain->domid);
+		if (rc) {
+			LOG_ERR("Failed to destroy domain#%u (rc=%d)", domain->domid, rc);
+		}
+
+		sys_dlist_remove(&domain->node);
+		--dom_num;
+		k_free(domain);
+	}
+	k_mutex_unlock(&dl_mutex);
 }
 
 struct xen_domain_cfg *domain_find_config(const char *name)
@@ -827,6 +868,7 @@ int domain_create(struct xen_domain_cfg *domcfg, uint32_t domid)
 	}
 
 	k_mutex_lock(&dl_mutex, K_FOREVER);
+	domain->refcount = 1;
 	sys_dnode_init(&domain->node);
 	sys_dlist_append(&domain_list, &domain->node);
 	++dom_num;
@@ -854,65 +896,28 @@ int domain_create(struct xen_domain_cfg *domcfg, uint32_t domid)
 
 int domain_destroy(uint32_t domid)
 {
-	int rc, err = 0;
 	struct xen_domain *domain = NULL;
 
-	domain = domid_to_domain(domid);
+	domain = get_domain(domid);
 	if (!domain) {
 		LOG_ERR("Domain with domid#%u is not found", domid);
 		/* Domain with requested domid is not present in list */
 		return -EINVAL;
 	}
 
-	if (domain->f_dom0less) {
-		LOG_ERR("dom0less domain#%u operation not supported", domid);
-		return -ENOTSUP;
-	}
-
-	rc = xs_remove_xenstore_backends(domain);
-	if (rc) {
-		LOG_ERR("Failed to remove_xenstore_backends domain#%u (rc=%d)", domain->domid, rc);
-		err = rc;
-	}
+	/* Call put domain twice to drop the original reference and trigger freeing */
+	put_domain(domain);
+	put_domain(domain);
 
-	rc = stop_domain_stored(domain);
-	if (rc) {
-		LOG_ERR("Failed to stop domain#%u store (rc=%d)", domain->domid, rc);
-		err = rc;
-	}
-
-	xs_deinitialize_domain_xenstore(domid);
-
-#ifdef CONFIG_XEN_CONSOLE_SRV
-	rc = xen_stop_domain_console(domain);
-	if (rc) {
-		LOG_ERR("Failed to stop domain#%u console (rc=%d)", domain->domid, rc);
-		err = rc;
-	}
-#endif
-
-	rc = xen_domctl_destroydomain(domid);
-	if (rc) {
-		LOG_ERR("Failed to destroy domain#%u (rc=%d)", domain->domid, rc);
-		err = rc;
-	}
-
-	k_mutex_lock(&dl_mutex, K_FOREVER);
-	sys_dlist_remove(&domain->node);
-	--dom_num;
-	k_mutex_unlock(&dl_mutex);
-
-	k_free(domain);
-
-	return err;
+	return 0;
 }
 
 int domain_pause(uint32_t domid)
 {
 	int rc;
 	struct xen_domain *domain = NULL;
 
-	domain = domid_to_domain(domid);
+	domain = get_domain(domid);
 	if (!domain) {
 		LOG_ERR("Domain with domid#%u is not found", domid);
 		/* Domain with requested domid is not present in list */
@@ -923,6 +928,7 @@ int domain_pause(uint32_t domid)
 	if (rc) {
 		LOG_ERR("domain:%u pause failed (%d)", domid, rc);
 	}
+	put_domain(domain);
 
 	return rc;
 }
@@ -932,7 +938,7 @@ int domain_unpause(uint32_t domid)
 	struct xen_domain *domain = NULL;
 	int rc;
 
-	domain = domid_to_domain(domid);
+	domain = get_domain(domid);
 	if (!domain) {
 		LOG_ERR("Domain with domid#%u is not found", domid);
 		/* Domain with requested domid is not present in list */
@@ -944,6 +950,7 @@ int domain_unpause(uint32_t domid)
 		LOG_ERR("domain:%u unpause failed (%d)", domid, rc);
 	}
 
+	put_domain(domain);
 	return rc;
 }
 
@@ -954,7 +961,7 @@ int domain_post_create(const struct xen_domain_cfg *domcfg, uint32_t domid)
 	struct backends_state *bs = NULL;
 	const struct backend_configuration *bc = NULL;
 
-	domain = domid_to_domain(domid);
+	domain = get_domain(domid);
 	bs = &domain->back_state;
 	bc = &domcfg->back_cfg;
 
@@ -984,9 +991,11 @@ int domain_post_create(const struct xen_domain_cfg *domcfg, uint32_t domid)
 		}
 	}
 
+	put_domain(domain);
 	return 0;
 
 deinit:
+	put_domain(domain);
 	LOG_ERR("Failed to initialize xenstore for domid#%u (rc=%d)", domid, rc);
 	domain_destroy(domid);
 	return rc;

diff --git a/xen-shell-cmd/src/xen_cmds.c b/xen-shell-cmd/src/xen_cmds.c
@@ -211,7 +211,7 @@ int domu_console_attach(const struct shell *shell, size_t argc, char **argv)
 		return -EINVAL;
 	}
 
-	domain = domid_to_domain(domid);
+	domain = get_domain(domid);
 	if (!domain) {
 		shell_error(shell, "domid#%u is not found", domid);
 		/* Domain with requested domid is not present in list */