[PATCH v5] x86/amd_node: Fix resource leak in amd_smn_init() error path

[PATCH v5] x86/amd_node: Fix resource leak in amd_smn_init() error path

[Ilan Bonneau-Zamon]

In amd_smn_init(), if an error occurs after reserving PCI config regions,
the already reserved regions are left allocated.

Unwind the setup properly in reverse order using traditional error labels
and goto statements, ensuring proper cleanup on error exit paths.

Fixes: 83518453074d ("x86/amd_node: Add SMN offsets to exclusive region access")
Signed-off-by: Ilan Bonneau-Zamon <ilan.bonneauzamon@gmail.com>
---
Changes in v5:
- Drop __free() cleanup helpers and custom release logic, switching to
  traditional goto cleanup pattern (as suggested by Borislav Petkov).
- Fix changelog attribution regarding brace removal (noted by Mario Limonciello).
- Update author sign-off to use legal name.

Changes in v4:
- Remove unnecessary braces around the kzalloc_objs() error check.
---
 arch/x86/kernel/amd_node.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
index b7926ba3610a..32038bd8e243 100644
--- a/arch/x86/kernel/amd_node.c
+++ b/arch/x86/kernel/amd_node.c
@@ -248,10 +248,19 @@ static int __init amd_smn_enable_dfs(char *str)
 }
 __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
 
+static void amd_smn_release_config_regions(struct pci_dev *root, u16 num_roots)
+{
+	while (num_roots && (root = get_next_root(root))) {
+		pci_release_config_region(root, 0, PCI_CFG_SPACE_SIZE);
+		num_roots--;
+	}
+}
+
 static int __init amd_smn_init(void)
 {
-	u16 count, num_roots, roots_per_node, node, num_nodes;
-	struct pci_dev *root __free(pci_dev_put) = NULL;
+	u16 count, num_roots = 0, roots_per_node, node, num_nodes;
+	struct pci_dev *root = NULL;
+	int ret;
 
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
 		return 0;
@@ -261,7 +270,6 @@ static int __init amd_smn_init(void)
 	if (amd_roots)
 		return 0;
 
-	num_roots = 0;
 	while ((root = get_next_root(root))) {
 		pci_dbg(root, "Reserving PCI config space\n");
 
@@ -273,7 +281,8 @@ static int __init amd_smn_init(void)
 		 */
 		if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
 			pci_err(root, "Failed to reserve config space\n");
-			return -EEXIST;
+			ret = -EEXIST;
+			goto err_release_regions;
 		}
 
 		num_roots++;
@@ -286,8 +295,10 @@ static int __init amd_smn_init(void)
 
 	num_nodes = amd_num_nodes();
 	amd_roots = kzalloc_objs(*amd_roots, num_nodes);
-	if (!amd_roots)
-		return -ENOMEM;
+	if (!amd_roots) {
+		ret = -ENOMEM;
+		goto err_release_regions;
+	}
 
 	roots_per_node = num_roots / num_nodes;
 	if (!roots_per_node) {
@@ -298,6 +309,7 @@ static int __init amd_smn_init(void)
 
 	count = 0;
 	node = 0;
+	root = NULL;
 	while (node < num_nodes && (root = get_next_root(root))) {
 		/* Use one root for each node and skip the rest. */
 		if (count++ % roots_per_node)
@@ -316,6 +328,10 @@ static int __init amd_smn_init(void)
 	}
 
 	return 0;
+
+err_release_regions:
+	amd_smn_release_config_regions(NULL, num_roots);
+	return ret;
 }
 
 fs_initcall(amd_smn_init);
-- 
2.53.0

Re: [PATCH v5] x86/amd_node: Fix resource leak in amd_smn_init() error path

[Yazen Ghannam]

On Wed, Sep 16, 2026 at 01:59:54PM +0200, Ilan Bonneau-Zamon wrote:
> In amd_smn_init(), if an error occurs after reserving PCI config regions,
> the already reserved regions are left allocated.
> 
> Unwind the setup properly in reverse order using traditional error labels
> and goto statements, ensuring proper cleanup on error exit paths.
> 

What is wrong with keeping these regions reserved?

The goal of reserving the regions is to prevent user space from using
them. Why should this be backed out?

Maybe we should *not* exit if a reservation fails? Rather, we should
unconditionally attempt to reserve all of them, and warn if reservation
fails on a particular device (but continue iterating).

Thanks,
Yazen