riscv: Prevent a bad reference count on CPU nodes
PATCH 1/1 riscv: Prevent a bad reference count on CPU nodes
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call 'of_node_put' for the node that was acquired.
Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an 'of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.
Moreover, the previous function did not check for errors when acquiring the device node, so a return -ENOENT has been added for that case.
---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.
arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
arch/riscv/kernel/cacheinfo.c +5 -2
@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {
@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" ))
--
2.46.0
Quoted 5 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.
Quoted 8 lines
Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.
LGTM,
Quoted 6 lines
Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.
Right, it is not a fix per say and hence not a stable material as ACPI is not accessing the node pointer.
Signature · 2 lines
Regards, Sudeep
Hi Miquel,
Quoted 38 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {
arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 105
Quoted 7 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +
It is necessary because the caller of populate_cache_leaves has a return value judgment.
So, Reviewed-by: Yunhui Cui <[email protected]>
Quoted 6 lines
arch/riscv/kernel/cacheinfo.c +0
-1if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" )) --2.46.0
Thanks,
Yunhui
Quoted 37 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {
arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 108
Quoted 10 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" ))
Gently ping :)
Could you take a look at this fix?
Thanks,
Miquel
Quoted 52 lines · Miquel Sabaté Solà
Quoted 37 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 108
Quoted 10 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" ))Gently ping :)
Quoted 4 lines
Could you take a look at this fix?
Thanks,
Miquel
Hello,
Would it make sense to have this fix for rc3?
Thanks,
Miquel
Hi Miquel,
Quoted 53 lines · Miquel Sabaté Solà
Quoted 51 lines · Miquel Sabaté Solà
Quoted 37 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 108
Quoted 10 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" ))Gently ping :)
Quoted 7 lines
Quoted 4 lines
Could you take a look at this fix?
Thanks,
MiquelHello,
Would it make sense to have this fix for rc3?
Sorry for the late response. It probably won't make it to rc3 but I'll make sure it will in rc4 :)
First:
And it needs the following Fixes tag (but no need to send a new version, b4 will pick it up):
And about ccing stable, I'm not sure what could be the impact of this bad reference count (some warnings could appear, etc...) so as it is a small patch, I think it's worth backporting to stable.
Thanks,
Alex
Quoted 8 lines
Thanks,
Miquel_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Quoted 78 lines · Alexandre Ghiti
Hi Miquel,
Quoted 53 lines · Miquel Sabaté Solà
Quoted 51 lines · Miquel Sabaté Solà
Quoted 37 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 108
Quoted 10 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +if (of_property_read_bool (np,"cache-size" ))ci_leaf_init (this_leaf++,CACHE_TYPE_UNIFIED , level);if (of_property_read_bool (np,"i-cache-size" ))Gently ping :)
Quoted 7 lines
Quoted 4 lines
Could you take a look at this fix?
Thanks,
MiquelHello,
Would it make sense to have this fix for rc3?
Sorry for the late response. It probably won't make it to rc3 but I'll make sure it will in rc4 :)
First:
Reviewed-by Alexandre Ghiti <[email protected]>And it needs the following Fixes tag (but no need to send a new version, b4 will pick it up):
Fixes 604f32ea6909 ("riscv: cacheinfo: initialize cacheinfo's level and typefrom ACPI PPTT")
Quoted 17 lines
And about ccing stable, I'm not sure what could be the impact of this bad reference count (some warnings could appear, etc...) so as it is a small patch, I think it's worth backporting to stable.
Thanks,
Alex
Quoted 8 lines
Thanks,
Miquel_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Nice, thank you!
Quoted 37 lines · Miquel Sabaté Solà
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.Moreover, the previous function did not check for errors when acquiring the device node, so a return
-ENOENThas been added for that case.Signed-off-by Miquel Sabaté Solà <[email protected]>---
I was wondering if this should also be sent to stable, but I have not seen a report on it, and this is not responsible for an oops or anything like that. So in the end I decided not to, but maybe you consider otherwise.arch/riscv/kernel/cacheinfo.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)arch/riscv/kernel/cacheinfo.c +1
-2@@ -75,8 +75,7 @@ int populate_cache_leaves(unsigned int cpu) {struct cpu_cacheinfo *this_cpu_ci =get_cpu_cacheinfo (cpu);struct cacheinfo *this_leaf = this_cpu_ci->info_list; -struct device_node *np =of_cpu_device_node_get (cpu); -struct device_node *prev =NULL ; +struct device_node *np, *prev;int levels =1 , level =1 ;if (!acpi_disabled) {
arch/riscv/kernel/cacheinfo.c int populate_cache_leaves(unsigned int cpu) · line 105
Quoted 7 lines
arch/riscv/kernel/cacheinfo.c +4
-0@@ -100,6 +99,10 @@ int populate_cache_leaves(unsigned int cpu)return 0 ; } + np =of_cpu_device_node_get (cpu); +if (!np) +return -ENOENT ; +
LGTM.
Thanks,
Sunil
This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <[email protected]>:
Hello:
This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:
Quoted 11 lines · On Fri, 13 Sep 2024 10:00:52 +0200 you wrote:
When populating cache leaves we previously fetched the CPU device node at the very beginning. But when ACPI is enabled we go through a specific branch which returns early and does not call '
of_node_put' for the node that was acquired.Since we are not using a CPU device node for the ACPI code anyways, we can simply move the initialization of it just passed the ACPI block, and we are guaranteed to have an '
of_node_put' call for the acquired node. This prevents a bad reference count of the CPU device node.[...]
Here is the summary with links:
- riscv: Prevent a bad reference count on CPU nodes
https://git.kernel.org/riscv/c/9510c5b0db36
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html