* 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-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
* 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