| Message ID | 20210423191236.265996-1-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | clk: Skip clk provider registration when np is NULL | expand |
On 4/23/21 10:12 PM, 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_provider() when calling s/of_clk_add_provider()/of_clk_add_hw_provider() > 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") > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > This would be the second approach, where we don't return an error when > one calls devm_of_clk_add_hw_provider with a NULL of_node, but instead > we just return 0 and skip the logic in the core and the drivers. > > drivers/clk/clk.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index e2ec1b745243..5d10da3519ac 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -4540,6 +4540,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; > @@ -4579,6 +4582,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; > @@ -4676,6 +4682,9 @@ void of_clk_del_provider(struct device_node *np) > { > struct of_clk_provider *cp; > > + if (!np) > + return 0; > + > mutex_lock(&of_clk_mutex); > list_for_each_entry(cp, &of_clk_providers, link) { > if (cp->node == np) { >
Quoting Tudor Ambarus (2021-04-23 12:12:36) > 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_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") > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- Please don't send patches as replies to previous threads. It makes it harder to find the patch at a glance of all threads. It also seems to be a Fixes: 3c9ea42802a1 ("clk: Mark fwnodes when their clock provider is added/removed") so can you please have both Fixes tags? > This would be the second approach, where we don't return an error when > one calls devm_of_clk_add_hw_provider with a NULL of_node, but instead > we just return 0 and skip the logic in the core and the drivers. With the Fixes tag updated please send To: gregkh@ to pick up as the problematic patch (6579c8d97ad7) is in the driver tree and not the clk tree, and add my tag Reviewed-by: Stephen Boyd <sboyd@kernel.org>
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index e2ec1b745243..5d10da3519ac 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4540,6 +4540,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; @@ -4579,6 +4582,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; @@ -4676,6 +4682,9 @@ void of_clk_del_provider(struct device_node *np) { struct of_clk_provider *cp; + if (!np) + return 0; + mutex_lock(&of_clk_mutex); list_for_each_entry(cp, &of_clk_providers, link) { if (cp->node == np) {
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_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") Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- This would be the second approach, where we don't return an error when one calls devm_of_clk_add_hw_provider with a NULL of_node, but instead we just return 0 and skip the logic in the core and the drivers. drivers/clk/clk.c | 9 +++++++++ 1 file changed, 9 insertions(+)