[PATCH 1/2] ia64: topology: Fix an error handling path in cache_add_dev()

[PATCH 1/2] ia64: topology: Fix an error handling path in cache_add_dev()

[Christophe JAILLET]

If kobject_init_and_add()fails, kobject_put() needs to be called.
Add the missing call which is already there a few lines below in another
error handling path.

Fixes: f19180056ea0 ("[IA64] Export cpu cache info by sysfs")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is provided as-is and is not even compile tested. I don't have the
cross-building toolchain for that.
---
 arch/ia64/kernel/topology.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/ia64/kernel/topology.c b/arch/ia64/kernel/topology.c
index 94a848b06f15..6adb84f05cbb 100644
--- a/arch/ia64/kernel/topology.c
+++ b/arch/ia64/kernel/topology.c
@@ -354,6 +354,7 @@ static int cache_add_dev(unsigned int cpu)
 				      &cache_ktype_percpu_entry, &sys_dev->kobj,
 				      "%s", "cache");
 	if (unlikely(retval < 0)) {
+		kobject_put(&all_cpu_cache_info[cpu].kobj);
 		cpu_cache_sysfs_exit(cpu);
 		return retval;
 	}
-- 
2.32.0

[PATCH 2/2] ia64: topology: Slightly simplify code

[Christophe JAILLET]

cpu_cache_sysfs_exit() already clears '&all_cpu_cache_info[cpu].kobj'. So
there is no need to do it twice.

Remove the redundant memset() and slightly simplify code.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is provided as-is and is not even compile tested. I don't have the
cross-building toolchain for that.
---
 arch/ia64/kernel/topology.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/ia64/kernel/topology.c b/arch/ia64/kernel/topology.c
index 6adb84f05cbb..df58df614873 100644
--- a/arch/ia64/kernel/topology.c
+++ b/arch/ia64/kernel/topology.c
@@ -387,12 +387,8 @@ static int cache_remove_dev(unsigned int cpu)
 	for (i = 0; i < all_cpu_cache_info[cpu].num_cache_leaves; i++)
 		kobject_put(&(LEAF_KOBJECT_PTR(cpu,i)->kobj));
 
-	if (all_cpu_cache_info[cpu].kobj.parent) {
+	if (all_cpu_cache_info[cpu].kobj.parent)
 		kobject_put(&all_cpu_cache_info[cpu].kobj);
-		memset(&all_cpu_cache_info[cpu].kobj,
-			0,
-			sizeof(struct kobject));
-	}
 
 	cpu_cache_sysfs_exit(cpu);
 
-- 
2.32.0

Re: [PATCH 1/2] ia64: topology: Fix an error handling path in cache_add_dev()

[John Paul Adrian Glaubitz]

Hi Christophe!

On 3/28/22 21:07, Christophe JAILLET wrote:
> If kobject_init_and_add()fails, kobject_put() needs to be called.
> Add the missing call which is already there a few lines below in another
> error handling path.
> 
> Fixes: f19180056ea0 ("[IA64] Export cpu cache info by sysfs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks for your patches. There is currently no maintainer for ia64, so the patches
would have to go through Andrew Morton's tree.

However, I can test the patches and verify they don't break anything.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH 1/2] ia64: topology: Fix an error handling path in cache_add_dev()

[Christophe JAILLET]

Le 28/03/2022 à 21:16, John Paul Adrian Glaubitz a écrit :
> Hi Christophe!
> 
> On 3/28/22 21:07, Christophe JAILLET wrote:
>> If kobject_init_and_add()fails, kobject_put() needs to be called.
>> Add the missing call which is already there a few lines below in another
>> error handling path.
>>
>> Fixes: f19180056ea0 ("[IA64] Export cpu cache info by sysfs")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> Thanks for your patches. There is currently no maintainer for ia64, so the patches
> would have to go through Andrew Morton's tree.
> 
> However, I can test the patches and verify they don't break anything.
> 
> Adrian
> 

Hi,

digging deeper for other potential same issues in other file, I don't 
think that this patch is needed, and I don't think that it fixes anything.

The "name" of this kobject is "%s", "cache".
So nothing needs to be freed for that because kstrdup_const() will be used.

This kobject has no .release function.

If the add() part of kobject_init_and_add(), then 'state_in_sysfs' will 
still be 0, so nothing needs to be released for that either.


So, adding a kobject_put() would just be a no-op here (if I understand 
correctly).

I've been puzzled by the kobject_put() later, but in this case, _add() 
has already succeeded and state_in_sysfs=1 and the call is needed.


For the other patch, it is just a clean-up. Based on Wikipedia, IA64 is 
discontinued, so such clean-up does not make that much sense either.
(on the other hand, it should be eay to review and apply :) )


I don't think you need to spent time on it. Sorry for the noise.

CJ

Re: [PATCH 1/2] ia64: topology: Fix an error handling path in cache_add_dev()

[John Paul Adrian Glaubitz]

Hi Christophe!

On 3/28/22 21:45, Christophe JAILLET wrote:
> For the other patch, it is just a clean-up. Based on Wikipedia, IA64 is
> discontinued, so such clean-up does not make that much sense either.
> (on the other hand, it should be eay to review and apply :) )

ia64 is still supported in Debian and Gentoo, so any kind of clean-up and
improvement of the kernel code is very welcome.

> I don't think you need to spent time on it. Sorry for the noise.

You didn't make any noise. If you have some improvements, I'm happy to test
them on my ia64 test system.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913