[PATCH v4] node: fix error handling in node_init_node_access

[PATCH v4] node: fix error handling in node_init_node_access

[Zhi Song]

dev_set_name() allocates new space to dev->name if it allocates
successfully. But if we failed to allocate space, there won't be any
new space for dev->name. Therefore, there's no need for calling
kfree_const(dev->kobj.name) in dev_set_name()'s error handling.

If we failed to calling device_register(dev), we just need to call
put_device(dev) which will do access_node freeing, kobj.name freeing
and other cleanup.

Fixes: 08d9dbe72b1f ("node: Link memory nodes to their compute nodes")
Signed-off-by: Zhi Song <zhi.song@bytedance.com>
---
V1 -> V2: Fix up the changelog text correct.
V2 -> V3: Add a fixes tag line specifying the commit where this bug was
introduced.
V3 -> V4: Fix the error handling for dev_set_name() and
device_register() to perform cleanup correctly.
---
 drivers/base/node.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 0ac6376ef7a1..0946931f113d 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -144,20 +144,19 @@ static struct node_access_nodes *node_init_node_access(struct node *node,
 	dev->parent = &node->dev;
 	dev->release = node_access_release;
 	dev->groups = node_access_node_groups;
-	if (dev_set_name(dev, "access%u", access))
-		goto free;
+	if (dev_set_name(dev, "access%u", access)) {
+		kfree(access_node);
+		return NULL;
+	}
 
-	if (device_register(dev))
-		goto free_name;
+	if (device_register(dev)) {
+		put_device(dev);
+		return NULL;
+	}
 
 	pm_runtime_no_callbacks(dev);
 	list_add_tail(&access_node->list_node, &node->access_list);
 	return access_node;
-free_name:
-	kfree_const(dev->kobj.name);
-free:
-	kfree(access_node);
-	return NULL;
 }
 
 #ifdef CONFIG_HMEM_REPORTING
-- 
2.30.2

Re: [PATCH v4] node: fix error handling in node_init_node_access

[Greg KH]

On Wed, Jun 22, 2022 at 01:16:23AM +0800, Zhi Song wrote:
> dev_set_name() allocates new space to dev->name if it allocates
> successfully. But if we failed to allocate space, there won't be any
> new space for dev->name. Therefore, there's no need for calling
> kfree_const(dev->kobj.name) in dev_set_name()'s error handling.

Can you actually trigger a failure in dev_set_name()?  I ask as we don't
seem to check this anywhere in the driver core, and we should either
just not care (as it really can not fail), or fix up all instances of
that failure.

> If we failed to calling device_register(dev), we just need to call
> put_device(dev) which will do access_node freeing, kobj.name freeing
> and other cleanup.

That is a separate issue, and should be a separate change, right?

thanks,

greg k-h