[PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[Donet Tom]

If register_node() returns an error, it is not handled correctly.
The function will proceed further and try to register CPUs under the
node, which is not correct.

So, in this patch, if register_node() returns an error, we return
immediately from the function.

Fixes: 76b67ed9dce6 ("[PATCH] node hotplug: register cpu: remove node struct")
Signed-off-by: Donet Tom <donettom@linux.ibm.com>
---
v1 -> v2
Made the changes based on Oscar’s review comments.

v1 - https://lore.kernel.org/all/20250702112856.295176-1-donettom@linux.ibm.com/
---
 drivers/base/node.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index c65b4917794e..1608816de67f 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -883,6 +883,11 @@ int register_one_node(int nid)
 	node_devices[nid] = node;
 
 	error = register_node(node_devices[nid], nid);
+	if (error) {
+		node_devices[nid] = NULL;
+		kfree(node);
+		return error;
+	}
 
 	/* link cpu under this node */
 	for_each_present_cpu(cpu) {
-- 
2.50.1

Re: [PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[David Hildenbrand]

On 22.08.25 10:48, Donet Tom wrote:
> If register_node() returns an error, it is not handled correctly.
> The function will proceed further and try to register CPUs under the
> node, which is not correct.
> 
> So, in this patch, if register_node() returns an error, we return
> immediately from the function.
> 
> Fixes: 76b67ed9dce6 ("[PATCH] node hotplug: register cpu: remove node struct")
> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[Chris Mason]

On Fri, 22 Aug 2025 14:18:45 +0530 Donet Tom <donettom@linux.ibm.com> wrote:

> If register_node() returns an error, it is not handled correctly.
> The function will proceed further and try to register CPUs under the
> node, which is not correct.
> 
> So, in this patch, if register_node() returns an error, we return
> immediately from the function.
> 
> Fixes: 76b67ed9dce6 ("[PATCH] node hotplug: register cpu: remove node struct")
> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
> ---
> v1 -> v2
> Made the changes based on Oscar’s review comments.
> 
> v1 - https://lore.kernel.org/all/20250702112856.295176-1-donettom@linux.ibm.com/
> ---
>  drivers/base/node.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index c65b4917794e..1608816de67f 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -883,6 +883,11 @@ int register_one_node(int nid)
>  	node_devices[nid] = node;
>  
>  	error = register_node(node_devices[nid], nid);
> +	if (error) {
> +		node_devices[nid] = NULL;
> +		kfree(node);
> +		return error;
> +	}

Can this cause a double-free? Looking at register_node(), when 
device_register() fails, it calls put_device(&node->dev). The put_device() 
call triggers node_device_release() which does kfree(to_node(dev)), freeing
the entire node structure. So when register_node() returns an error, the 
node memory is already freed, but this code calls kfree(node) again on the
same memory.

The call chain is: register_node()->device_register() fails->
put_device()->node_device_release()->kfree(to_node(dev)).

[ This came from automated patch review, but it looks real to me ]

-chris

Re: [PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[Donet Tom]

On 9/17/25 7:15 PM, Chris Mason wrote:
> On Fri, 22 Aug 2025 14:18:45 +0530 Donet Tom <donettom@linux.ibm.com> wrote:
>
>> If register_node() returns an error, it is not handled correctly.
>> The function will proceed further and try to register CPUs under the
>> node, which is not correct.
>>
>> So, in this patch, if register_node() returns an error, we return
>> immediately from the function.
>>
>> Fixes: 76b67ed9dce6 ("[PATCH] node hotplug: register cpu: remove node struct")
>> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
>> ---
>> v1 -> v2
>> Made the changes based on Oscar’s review comments.
>>
>> v1 - https://lore.kernel.org/all/20250702112856.295176-1-donettom@linux.ibm.com/
>> ---
>>   drivers/base/node.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index c65b4917794e..1608816de67f 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -883,6 +883,11 @@ int register_one_node(int nid)
>>   	node_devices[nid] = node;
>>   
>>   	error = register_node(node_devices[nid], nid);
>> +	if (error) {
>> +		node_devices[nid] = NULL;
>> +		kfree(node);
>> +		return error;
>> +	}
> Can this cause a double-free? Looking at register_node(), when
> device_register() fails, it calls put_device(&node->dev). The put_device()
> call triggers node_device_release() which does kfree(to_node(dev)), freeing
> the entire node structure. So when register_node() returns an error, the
> node memory is already freed, but this code calls kfree(node) again on the
> same memory.
>
> The call chain is: register_node()->device_register() fails->
> put_device()->node_device_release()->kfree(to_node(dev)).


Thank you for pointing this out. I will address it and send a v3.


>
> [ This came from automated patch review, but it looks real to me ]
>
> -chris
>

Re: [PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[Andrew Morton]

On Wed, 17 Sep 2025 20:25:48 +0530 Donet Tom <donettom@linux.ibm.com> wrote:

> > Can this cause a double-free? Looking at register_node(), when
> > device_register() fails, it calls put_device(&node->dev). The put_device()
> > call triggers node_device_release() which does kfree(to_node(dev)), freeing
> > the entire node structure. So when register_node() returns an error, the
> > node memory is already freed, but this code calls kfree(node) again on the
> > same memory.
> >
> > The call chain is: register_node()->device_register() fails->
> > put_device()->node_device_release()->kfree(to_node(dev)).
> 
> 
> Thank you for pointing this out. I will address it and send a v3.

This patch is now in mm.git's non-rebasing mm-stable branch, so no
replacements, please.

A standalone patch with 

	Fixes: 786eb990cfb7 ("drivers/base/node: handle error properly in register_one_node()")

is the way to go.

Re: [PATCH v2] drivers/base/node: Handle error properly in register_one_node()

[Donet Tom]

On 9/18/25 3:18 AM, Andrew Morton wrote:
> On Wed, 17 Sep 2025 20:25:48 +0530 Donet Tom <donettom@linux.ibm.com> wrote:
>
>>> Can this cause a double-free? Looking at register_node(), when
>>> device_register() fails, it calls put_device(&node->dev). The put_device()
>>> call triggers node_device_release() which does kfree(to_node(dev)), freeing
>>> the entire node structure. So when register_node() returns an error, the
>>> node memory is already freed, but this code calls kfree(node) again on the
>>> same memory.
>>>
>>> The call chain is: register_node()->device_register() fails->
>>> put_device()->node_device_release()->kfree(to_node(dev)).
>>
>> Thank you for pointing this out. I will address it and send a v3.
> This patch is now in mm.git's non-rebasing mm-stable branch, so no
> replacements, please.
>
> A standalone patch with
>
> 	Fixes: 786eb990cfb7 ("drivers/base/node: handle error properly in register_one_node()")
>
> is the way to go.


Sure Andrew .I will send it today.


>
>