linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] clk: Skip clk provider registration when np is NULL
@ 2021-04-26  6:56 Tudor Ambarus
  2021-04-26 18:17 ` Saravana Kannan
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Tudor Ambarus @ 2021-04-26  6:56 UTC (permalink / raw)
  To: gregkh, rafael, mturquette, sboyd, nsaenz, maxime, khilman,
	ulf.hansson, len.brown, pavel, robh+dt, frowand.list, maz, tglx,
	saravanak, geert, nsaenzjulienne, linux, guillaume.tucker
  Cc: linux-clk, linux-kernel, corbet, nicolas.ferre, claudiu.beznea,
	linux-doc, linux-pm, devicetree, linux-acpi, kernel-team,
	linux-rpi-kernel, Tudor Ambarus, Marek Szyprowski

commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
revealed that clk/bcm/clk-raspberrypi.c driver calls
devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
NULL pointer dereference in of_clk_add_hw_provider() when calling
fwnode_dev_initialized().

Returning 0 is reducing the if conditions in driver code and is being
consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
is disabled. The downside is that drivers will maybe register clkdev lookups
when they don't need to and waste some memory.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
v2:
- s/return 0;/return; in void of_clk_del_provider()
- add second fixes tag and Stephen's R-b tag
The opinions on whether to return an error or zero were split. Returning 0
and skipping the logic was considered safer as we don't know for sure if
other drivers are affected. See:
https://lore.kernel.org/lkml/d24bebc5-0f78-021f-293f-e58defa32531@samsung.com/
https://lore.kernel.org/lkml/20210423171335.262316-1-tudor.ambarus@microchip.com/

 drivers/clk/clk.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a3b30f7de2ef..b47460b40d14 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4552,6 +4552,9 @@ int of_clk_add_provider(struct device_node *np,
 	struct of_clk_provider *cp;
 	int ret;
 
+	if (!np)
+		return 0;
+
 	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
 	if (!cp)
 		return -ENOMEM;
@@ -4591,6 +4594,9 @@ int of_clk_add_hw_provider(struct device_node *np,
 	struct of_clk_provider *cp;
 	int ret;
 
+	if (!np)
+		return 0;
+
 	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
 	if (!cp)
 		return -ENOMEM;
@@ -4688,6 +4694,9 @@ void of_clk_del_provider(struct device_node *np)
 {
 	struct of_clk_provider *cp;
 
+	if (!np)
+		return;
+
 	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(cp, &of_clk_providers, link) {
 		if (cp->node == np) {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] clk: Skip clk provider registration when np is NULL
  2021-04-26  6:56 [PATCH v2] clk: Skip clk provider registration when np is NULL Tudor Ambarus
@ 2021-04-26 18:17 ` Saravana Kannan
  2021-04-27 14:57 ` nicolas saenz julienne
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Saravana Kannan @ 2021-04-26 18:17 UTC (permalink / raw)
  To: Tudor Ambarus
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Michael Turquette,
	Stephen Boyd, nsaenz, maxime, Kevin Hilman, Ulf Hansson, Brown,
	Len, Pavel Machek, Rob Herring, Frank Rowand, Marc Zyngier,
	Thomas Gleixner, Geert Uytterhoeven, Nicolas Saenz Julienne,
	Guenter Roeck, guillaume.tucker, linux-clk, LKML,
	Jonathan Corbet, Nicolas Ferre, Claudiu Beznea,
	Linux Doc Mailing List, Linux PM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	ACPI Devel Maling List, Android Kernel Team,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
	<devicetre e@vger.kernel.org>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Android Kernel Team <kernel-team@android.com>,
	linux-rpi-kernel, Marek Szyprowski

On Sun, Apr 25, 2021 at 11:56 PM Tudor Ambarus
<tudor.ambarus@microchip.com> wrote:
>
> commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> revealed that clk/bcm/clk-raspberrypi.c driver calls
> devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
> NULL pointer dereference in of_clk_add_hw_provider() when calling
> fwnode_dev_initialized().
>
> Returning 0 is reducing the if conditions in driver code and is being
> consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
> is disabled. The downside is that drivers will maybe register clkdev lookups
> when they don't need to and waste some memory.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> ---

Reviewed-by: Saravana Kannan <saravanak@google.com>

-Saravana

> v2:
> - s/return 0;/return; in void of_clk_del_provider()
> - add second fixes tag and Stephen's R-b tag
> The opinions on whether to return an error or zero were split. Returning 0
> and skipping the logic was considered safer as we don't know for sure if
> other drivers are affected. See:
> https://lore.kernel.org/lkml/d24bebc5-0f78-021f-293f-e58defa32531@samsung.com/
> https://lore.kernel.org/lkml/20210423171335.262316-1-tudor.ambarus@microchip.com/
>
>  drivers/clk/clk.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a3b30f7de2ef..b47460b40d14 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4552,6 +4552,9 @@ int of_clk_add_provider(struct device_node *np,
>         struct of_clk_provider *cp;
>         int ret;
>
> +       if (!np)
> +               return 0;
> +
>         cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>         if (!cp)
>                 return -ENOMEM;
> @@ -4591,6 +4594,9 @@ int of_clk_add_hw_provider(struct device_node *np,
>         struct of_clk_provider *cp;
>         int ret;
>
> +       if (!np)
> +               return 0;
> +
>         cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>         if (!cp)
>                 return -ENOMEM;
> @@ -4688,6 +4694,9 @@ void of_clk_del_provider(struct device_node *np)
>  {
>         struct of_clk_provider *cp;
>
> +       if (!np)
> +               return;
> +
>         mutex_lock(&of_clk_mutex);
>         list_for_each_entry(cp, &of_clk_providers, link) {
>                 if (cp->node == np) {
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] clk: Skip clk provider registration when np is NULL
  2021-04-26  6:56 [PATCH v2] clk: Skip clk provider registration when np is NULL Tudor Ambarus
  2021-04-26 18:17 ` Saravana Kannan
@ 2021-04-27 14:57 ` nicolas saenz julienne
  2021-05-10 19:36 ` Guenter Roeck
  2021-05-10 20:20 ` Nathan Chancellor
  3 siblings, 0 replies; 6+ messages in thread
From: nicolas saenz julienne @ 2021-04-27 14:57 UTC (permalink / raw)
  To: Tudor Ambarus, gregkh, rafael, mturquette, sboyd, maxime,
	khilman, ulf.hansson, len.brown, pavel, robh+dt, frowand.list,
	maz, tglx, saravanak, geert, nsaenzjulienne, linux,
	guillaume.tucker
  Cc: linux-clk, linux-kernel, corbet, nicolas.ferre, claudiu.beznea,
	linux-doc, linux-pm, devicetree, linux-acpi, kernel-team,
	linux-rpi-kernel, Marek Szyprowski

On Mon, 2021-04-26 at 09:56 +0300, Tudor Ambarus wrote:
> commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> revealed that clk/bcm/clk-raspberrypi.c driver calls
> devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
> NULL pointer dereference in of_clk_add_hw_provider() when calling
> fwnode_dev_initialized().
> 
> Returning 0 is reducing the if conditions in driver code and is being
> consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
> is disabled. The downside is that drivers will maybe register clkdev lookups
> when they don't need to and waste some memory.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> ---

Reviewed-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Regards,
Nicolas


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] clk: Skip clk provider registration when np is NULL
  2021-04-26  6:56 [PATCH v2] clk: Skip clk provider registration when np is NULL Tudor Ambarus
  2021-04-26 18:17 ` Saravana Kannan
  2021-04-27 14:57 ` nicolas saenz julienne
@ 2021-05-10 19:36 ` Guenter Roeck
  2021-05-11  7:11   ` Greg KH
  2021-05-10 20:20 ` Nathan Chancellor
  3 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-05-10 19:36 UTC (permalink / raw)
  To: Tudor Ambarus
  Cc: gregkh, rafael, mturquette, sboyd, nsaenz, maxime, khilman,
	ulf.hansson, len.brown, pavel, robh+dt, frowand.list, maz, tglx,
	saravanak, geert, nsaenzjulienne, guillaume.tucker, linux-clk,
	linux-kernel, corbet, nicolas.ferre, claudiu.beznea, linux-doc,
	linux-pm, devicetree, linux-acpi, kernel-team, linux-rpi-kernel,
	Marek Szyprowski

On Mon, Apr 26, 2021 at 09:56:18AM +0300, Tudor Ambarus wrote:
> commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> revealed that clk/bcm/clk-raspberrypi.c driver calls
> devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
> NULL pointer dereference in of_clk_add_hw_provider() when calling
> fwnode_dev_initialized().
> 
> Returning 0 is reducing the if conditions in driver code and is being
> consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
> is disabled. The downside is that drivers will maybe register clkdev lookups
> when they don't need to and waste some memory.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v2:
> - s/return 0;/return; in void of_clk_del_provider()
> - add second fixes tag and Stephen's R-b tag
> The opinions on whether to return an error or zero were split. Returning 0
> and skipping the logic was considered safer as we don't know for sure if
> other drivers are affected. See:
> https://lore.kernel.org/lkml/d24bebc5-0f78-021f-293f-e58defa32531@samsung.com/
> https://lore.kernel.org/lkml/20210423171335.262316-1-tudor.ambarus@microchip.com/
> 
>  drivers/clk/clk.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a3b30f7de2ef..b47460b40d14 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4552,6 +4552,9 @@ int of_clk_add_provider(struct device_node *np,
>  	struct of_clk_provider *cp;
>  	int ret;
>  
> +	if (!np)
> +		return 0;
> +
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>  	if (!cp)
>  		return -ENOMEM;
> @@ -4591,6 +4594,9 @@ int of_clk_add_hw_provider(struct device_node *np,
>  	struct of_clk_provider *cp;
>  	int ret;
>  
> +	if (!np)
> +		return 0;
> +
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>  	if (!cp)
>  		return -ENOMEM;
> @@ -4688,6 +4694,9 @@ void of_clk_del_provider(struct device_node *np)
>  {
>  	struct of_clk_provider *cp;
>  
> +	if (!np)
> +		return;
> +
>  	mutex_lock(&of_clk_mutex);
>  	list_for_each_entry(cp, &of_clk_providers, link) {
>  		if (cp->node == np) {
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] clk: Skip clk provider registration when np is NULL
  2021-04-26  6:56 [PATCH v2] clk: Skip clk provider registration when np is NULL Tudor Ambarus
                   ` (2 preceding siblings ...)
  2021-05-10 19:36 ` Guenter Roeck
@ 2021-05-10 20:20 ` Nathan Chancellor
  3 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2021-05-10 20:20 UTC (permalink / raw)
  To: Tudor Ambarus
  Cc: gregkh, rafael, mturquette, sboyd, nsaenz, maxime, khilman,
	ulf.hansson, len.brown, pavel, robh+dt, frowand.list, maz, tglx,
	saravanak, geert, nsaenzjulienne, linux, guillaume.tucker,
	linux-clk, linux-kernel, corbet, nicolas.ferre, claudiu.beznea,
	linux-doc, linux-pm, devicetree, linux-acpi, kernel-team,
	linux-rpi-kernel, Marek Szyprowski

On Mon, Apr 26, 2021 at 09:56:18AM +0300, Tudor Ambarus wrote:
> commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> revealed that clk/bcm/clk-raspberrypi.c driver calls
> devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
> NULL pointer dereference in of_clk_add_hw_provider() when calling
> fwnode_dev_initialized().
> 
> Returning 0 is reducing the if conditions in driver code and is being
> consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
> is disabled. The downside is that drivers will maybe register clkdev lookups
> when they don't need to and waste some memory.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
> v2:
> - s/return 0;/return; in void of_clk_del_provider()
> - add second fixes tag and Stephen's R-b tag
> The opinions on whether to return an error or zero were split. Returning 0
> and skipping the logic was considered safer as we don't know for sure if
> other drivers are affected. See:
> https://lore.kernel.org/lkml/d24bebc5-0f78-021f-293f-e58defa32531@samsung.com/
> https://lore.kernel.org/lkml/20210423171335.262316-1-tudor.ambarus@microchip.com/
> 
>  drivers/clk/clk.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a3b30f7de2ef..b47460b40d14 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4552,6 +4552,9 @@ int of_clk_add_provider(struct device_node *np,
>  	struct of_clk_provider *cp;
>  	int ret;
>  
> +	if (!np)
> +		return 0;
> +
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>  	if (!cp)
>  		return -ENOMEM;
> @@ -4591,6 +4594,9 @@ int of_clk_add_hw_provider(struct device_node *np,
>  	struct of_clk_provider *cp;
>  	int ret;
>  
> +	if (!np)
> +		return 0;
> +
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>  	if (!cp)
>  		return -ENOMEM;
> @@ -4688,6 +4694,9 @@ void of_clk_del_provider(struct device_node *np)
>  {
>  	struct of_clk_provider *cp;
>  
> +	if (!np)
> +		return;
> +
>  	mutex_lock(&of_clk_mutex);
>  	list_for_each_entry(cp, &of_clk_providers, link) {
>  		if (cp->node == np) {
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] clk: Skip clk provider registration when np is NULL
  2021-05-10 19:36 ` Guenter Roeck
@ 2021-05-11  7:11   ` Greg KH
  0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2021-05-11  7:11 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Tudor Ambarus, rafael, mturquette, sboyd, nsaenz, maxime,
	khilman, ulf.hansson, len.brown, pavel, robh+dt, frowand.list,
	maz, tglx, saravanak, geert, nsaenzjulienne, guillaume.tucker,
	linux-clk, linux-kernel, corbet, nicolas.ferre, claudiu.beznea,
	linux-doc, linux-pm, devicetree, linux-acpi, kernel-team,
	linux-rpi-kernel, Marek Szyprowski

On Mon, May 10, 2021 at 12:36:45PM -0700, Guenter Roeck wrote:
> On Mon, Apr 26, 2021 at 09:56:18AM +0300, Tudor Ambarus wrote:
> > commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> > revealed that clk/bcm/clk-raspberrypi.c driver calls
> > devm_of_clk_add_hw_provider(), with a NULL dev->of_node, which resulted in a
> > NULL pointer dereference in of_clk_add_hw_provider() when calling
> > fwnode_dev_initialized().
> > 
> > Returning 0 is reducing the if conditions in driver code and is being
> > consistent with the CONFIG_OF=n inline stub that returns 0 when CONFIG_OF
> > is disabled. The downside is that drivers will maybe register clkdev lookups
> > when they don't need to and waste some memory.
> > 
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Fixes: 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added")
> > Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed")
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Now applied to my tree, thanks and sorry for the delay, I thought this
was going through the clk tree.

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-11  7:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26  6:56 [PATCH v2] clk: Skip clk provider registration when np is NULL Tudor Ambarus
2021-04-26 18:17 ` Saravana Kannan
2021-04-27 14:57 ` nicolas saenz julienne
2021-05-10 19:36 ` Guenter Roeck
2021-05-11  7:11   ` Greg KH
2021-05-10 20:20 ` Nathan Chancellor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).