* [PATCH 0/3] fix leaked of_node references in drivers/firmware @ 2019-04-17 2:44 Wen Yang 2019-04-17 2:44 ` Wen Yang ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel; +Cc: wang.yi59, Wen Yang The call to of_get_cpu_node/of_find_compatible_node/of_parse_phandle... returns a node pointer with refcount incremented thus it must be explicitly decremented after the last usage. We developed a coccinelle SmPL to detect drivers/firmware code and found some issues. This patch series fixes those issues. Wen Yang (3): firmware: arm_sdei: fix leaked of_node references firmware: psci: fix leaked of_node references firmware: stratix10-svc: fix leaked of_node references drivers/firmware/arm_sdei.c | 1 + drivers/firmware/psci.c | 4 +++- drivers/firmware/stratix10-svc.c | 14 ++++++++++---- 3 files changed, 14 insertions(+), 5 deletions(-) -- 2.9.5 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] firmware: arm_sdei: fix leaked of_node references 2019-04-17 2:44 [PATCH 0/3] fix leaked of_node references in drivers/firmware Wen Yang @ 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` [PATCH 3/3] firmware: stratix10-svc: " Wen Yang 2 siblings, 0 replies; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel; +Cc: wang.yi59, Wen Yang, James Morse, linux-arm-kernel In sdei_present_dt function, fw_np is obtained by calling of_find_node_by_name(), np is obtained by calling of_find_matching_node(), and the reference counts of those two device_nodes, fw_np and np, are increased. But when the function exits, only of_node_put is called on np, and fw_np's reference count is leaked. Detected by coccinelle with the following warnings: ./drivers/firmware/arm_sdei.c:1088:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function. ./drivers/firmware/arm_sdei.c:1091:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: James Morse <james.morse@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/firmware/arm_sdei.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index e6376f9..2faa329 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -1084,6 +1084,7 @@ static bool __init sdei_present_dt(void) return false; np = of_find_matching_node(fw_np, sdei_of_match); + of_node_put(fw_np); if (!np) return false; of_node_put(np); -- 2.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/3] firmware: arm_sdei: fix leaked of_node references @ 2019-04-17 2:44 ` Wen Yang 0 siblings, 0 replies; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel; +Cc: wang.yi59, James Morse, Wen Yang, linux-arm-kernel In sdei_present_dt function, fw_np is obtained by calling of_find_node_by_name(), np is obtained by calling of_find_matching_node(), and the reference counts of those two device_nodes, fw_np and np, are increased. But when the function exits, only of_node_put is called on np, and fw_np's reference count is leaked. Detected by coccinelle with the following warnings: ./drivers/firmware/arm_sdei.c:1088:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function. ./drivers/firmware/arm_sdei.c:1091:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: James Morse <james.morse@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/firmware/arm_sdei.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index e6376f9..2faa329 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -1084,6 +1084,7 @@ static bool __init sdei_present_dt(void) return false; np = of_find_matching_node(fw_np, sdei_of_match); + of_node_put(fw_np); if (!np) return false; of_node_put(np); -- 2.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] firmware: psci: fix leaked of_node references 2019-04-17 2:44 [PATCH 0/3] fix leaked of_node references in drivers/firmware Wen Yang @ 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` [PATCH 3/3] firmware: stratix10-svc: " Wen Yang 2 siblings, 0 replies; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel Cc: wang.yi59, Wen Yang, Mark Rutland, Lorenzo Pieralisi, linux-arm-kernel The call to of_find_matching_node_and_match returns a node pointer with refcount incremented thus it must be explicitly decremented after the last usage. 672 int __init psci_dt_init(void) 673 { 674 struct device_node *np; ... 678 np = of_find_matching_node_and_match(...); 679 680 if (!np || !of_device_is_available(np)) 682 return -ENODEV; ---> leaked here ... 686 return init_fn(np); ---> released here 687 } Detected by using coccinelle. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/firmware/psci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c80ec1d..e4143f8 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -677,8 +677,10 @@ int __init psci_dt_init(void) np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); - if (!np || !of_device_is_available(np)) + if (!np || !of_device_is_available(np)) { + of_node_put(np); return -ENODEV; + } init_fn = (psci_initcall_t)matched_np->data; return init_fn(np); -- 2.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] firmware: psci: fix leaked of_node references @ 2019-04-17 2:44 ` Wen Yang 0 siblings, 0 replies; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel Cc: wang.yi59, Mark Rutland, Lorenzo Pieralisi, Wen Yang, linux-arm-kernel The call to of_find_matching_node_and_match returns a node pointer with refcount incremented thus it must be explicitly decremented after the last usage. 672 int __init psci_dt_init(void) 673 { 674 struct device_node *np; ... 678 np = of_find_matching_node_and_match(...); 679 680 if (!np || !of_device_is_available(np)) 682 return -ENODEV; ---> leaked here ... 686 return init_fn(np); ---> released here 687 } Detected by using coccinelle. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/firmware/psci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c80ec1d..e4143f8 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -677,8 +677,10 @@ int __init psci_dt_init(void) np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); - if (!np || !of_device_is_available(np)) + if (!np || !of_device_is_available(np)) { + of_node_put(np); return -ENODEV; + } init_fn = (psci_initcall_t)matched_np->data; return init_fn(np); -- 2.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] firmware: stratix10-svc: fix leaked of_node references 2019-04-17 2:44 [PATCH 0/3] fix leaked of_node references in drivers/firmware Wen Yang 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` Wen Yang @ 2019-04-17 2:44 ` Wen Yang 2019-04-17 11:32 ` Nicolas Saenz Julienne 2 siblings, 1 reply; 8+ messages in thread From: Wen Yang @ 2019-04-17 2:44 UTC (permalink / raw) To: linux-kernel Cc: wang.yi59, Wen Yang, Greg Kroah-Hartman, Alan Tull, Richard Gong, Nicolas Saenz Julienne In stratix10_svc_init function, fw_np is obtained by calling of_find_node_by_name(), np is obtained by calling of_find_matching_node(), and the reference counts of those two device_nodes, fw_np and np, are increased. But when the function exits, only of_node_put is called on np, and fw_np's reference count is leaked. Detected by coccinelle with the following warnings: ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function. ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function. ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function. Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Alan Tull <atull@kernel.org> Cc: Richard Gong <richard.gong@intel.com> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Cc: linux-kernel@vger.kernel.org --- drivers/firmware/stratix10-svc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c index 6e65148..482a6bd 100644 --- a/drivers/firmware/stratix10-svc.c +++ b/drivers/firmware/stratix10-svc.c @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void) return -ENODEV; np = of_find_matching_node(fw_np, stratix10_svc_drv_match); - if (!np) - return -ENODEV; + if (!np) { + ret = -ENODEV; + goto out_put_fw_np; + } of_node_put(np); ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL); if (ret) - return ret; + goto out_put_fw_np; - return platform_driver_register(&stratix10_svc_driver); + ret = platform_driver_register(&stratix10_svc_driver); + +out_put_fw_np: + of_node_put(fw_np); + return ret; } static void __exit stratix10_svc_exit(void) -- 2.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] firmware: stratix10-svc: fix leaked of_node references 2019-04-17 2:44 ` [PATCH 3/3] firmware: stratix10-svc: " Wen Yang @ 2019-04-17 11:32 ` Nicolas Saenz Julienne 2019-04-18 1:54 ` [PATCH 3/3] firmware: stratix10-svc: fix leaked of_nodereferences wen.yang99 0 siblings, 1 reply; 8+ messages in thread From: Nicolas Saenz Julienne @ 2019-04-17 11:32 UTC (permalink / raw) To: Wen Yang, linux-kernel Cc: wang.yi59, Greg Kroah-Hartman, Alan Tull, Richard Gong [-- Attachment #1: Type: text/plain, Size: 2352 bytes --] On Wed, 2019-04-17 at 10:44 +0800, Wen Yang wrote: > In stratix10_svc_init function, fw_np is obtained by calling > of_find_node_by_name(), np is obtained by calling > of_find_matching_node(), and the reference counts of those > two device_nodes, fw_np and np, are increased. > But when the function exits, only of_node_put is called on np, > and fw_np's reference count is leaked. > > Detected by coccinelle with the following warnings: > ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put; > acquired a node pointer with refcount incremented on line 1014, but without a > corresponding object release within this function. > ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put; > acquired a node pointer with refcount incremented on line 1014, but without a > corresponding object release within this function. > ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put; > acquired a node pointer with refcount incremented on line 1014, but without a > corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Alan Tull <atull@kernel.org> > Cc: Richard Gong <richard.gong@intel.com> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Cc: linux-kernel@vger.kernel.org > --- > drivers/firmware/stratix10-svc.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10- > svc.c > index 6e65148..482a6bd 100644 > --- a/drivers/firmware/stratix10-svc.c > +++ b/drivers/firmware/stratix10-svc.c > @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void) > return -ENODEV; > > np = of_find_matching_node(fw_np, stratix10_svc_drv_match); Sorry but this patch isn't right, of_find_matching_node() will free the reference to fw_np internally. > - if (!np) > - return -ENODEV; > + if (!np) { > + ret = -ENODEV; > + goto out_put_fw_np; > + } > > of_node_put(np); > ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL); > if (ret) > - return ret; > + goto out_put_fw_np; Consequently and assuming I'm not missing something, I think fw_np shouldn't be used here as is. Regards, Nicolas [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] firmware: stratix10-svc: fix leaked of_nodereferences 2019-04-17 11:32 ` Nicolas Saenz Julienne @ 2019-04-18 1:54 ` wen.yang99 0 siblings, 0 replies; 8+ messages in thread From: wen.yang99 @ 2019-04-18 1:54 UTC (permalink / raw) To: nsaenzjulienne Cc: linux-kernel, wang.yi59, gregkh, atull, richard.gong, mdf, linux-fpga [-- Attachment #1.1: Type: text/plain, Size: 3694 bytes --] Hi Nicolas, > > In stratix10_svc_init function, fw_np is obtained by calling > > of_find_node_by_name(), np is obtained by calling > > of_find_matching_node(), and the reference counts of those > > two device_nodes, fw_np and np, are increased. > > But when the function exits, only of_node_put is called on np, > > and fw_np's reference count is leaked. > > > > Detected by coccinelle with the following warnings: > > ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put; > > acquired a node pointer with refcount incremented on line 1014, but without a > > corresponding object release within this function. > > ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put; > > acquired a node pointer with refcount incremented on line 1014, but without a > > corresponding object release within this function. > > ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put; > > acquired a node pointer with refcount incremented on line 1014, but without a > > corresponding object release within this function. > > > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Alan Tull <atull@kernel.org> > > Cc: Richard Gong <richard.gong@intel.com> > > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/firmware/stratix10-svc.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10- > > svc.c > > index 6e65148..482a6bd 100644 > > --- a/drivers/firmware/stratix10-svc.c > > +++ b/drivers/firmware/stratix10-svc.c > > @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void) > > return -ENODEV; > > > > np = of_find_matching_node(fw_np, stratix10_svc_drv_match); > > Sorry but this patch isn't right, of_find_matching_node() will free the > reference to fw_np internally. > > > - if (!np) > > - return -ENODEV; > > + if (!np) { > > + ret = -ENODEV; > > + goto out_put_fw_np; > > + } > > > > of_node_put(np); > > ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL); > > if (ret) > > - return ret; > > + goto out_put_fw_np; > > Consequently and assuming I'm not missing something, I think fw_np shouldn't be > used here as is. Ok, thank you very much for your comments. of_find_matching_node() will indeed free the reference to fw_np internally. We will continue to improve the SmPL rules based on your comments. In addition, we also checked it further and found that this commit may have a problem: 32a66374487b ("fpga: stratix10-soc: fix wrong of_node_put() in init function") drivers/fpga/stratix10-soc.c 500 static int __init s10_init(void) 501 { 502 struct device_node *fw_np; 503 struct device_node *np; 504 int ret; 505 506 fw_np = of_find_node_by_name(NULL, "svc"); 507 if (!fw_np) 508 return -ENODEV; 509 510 np = of_find_matching_node(fw_np, s10_of_match); -->fw_np released here 511 if (!np) 512 return -ENODEV; 513 514 of_node_put(np); 515 ret = of_platform_populate(fw_np, s10_of_match, NULL, NULL); -->fw_np shouldn't be used here 516 if (ret) 517 return ret; 518 519 return platform_driver_register(&s10_driver); 520 } We plan to add such SmPL rules to further check the current kernel code: after of_find_matching_node(node, ...), of_node_put(node) should not be called, and "node" should not be used (unless there is an extra of_node_get). -- Best wishes, Wen ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-18 1:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-17 2:44 [PATCH 0/3] fix leaked of_node references in drivers/firmware Wen Yang 2019-04-17 2:44 ` [PATCH 1/3] firmware: arm_sdei: fix leaked of_node references Wen Yang 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` [PATCH 2/3] firmware: psci: " Wen Yang 2019-04-17 2:44 ` Wen Yang 2019-04-17 2:44 ` [PATCH 3/3] firmware: stratix10-svc: " Wen Yang 2019-04-17 11:32 ` Nicolas Saenz Julienne 2019-04-18 1:54 ` [PATCH 3/3] firmware: stratix10-svc: fix leaked of_nodereferences wen.yang99
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.