| Message ID | 20210423171335.262316-1-tudor.ambarus@microchip.com |
|---|---|
| Headers | show |
| Series | clk: Do not register provider with a NULL dev->of_node | expand |
On Fri, Apr 23, 2021 at 10:14 AM 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. > > devm_of_clk_add_hw_provider() should not register the provider with > a NULL dev->of_node, as there is no of_node. Apart of the NULL pointer > dereference that will result when calling fwnode_dev_initialized() in > of_clk_add_hw_provider(), another problem is that when two drivers calling > of_clk_add_hw_provider() with np = NULL, their unregistration order is not > guaranteed to be correct. Avoid all the problems and just return -ENODEV > when the callers of devm_of_clk_add_hw_provider() use a NULL dev->of_node, > which seems the natural way to do. > > 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> > --- > drivers/clk/clk.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index e2ec1b745243..8b5077cc5e67 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -4634,11 +4634,10 @@ static struct device_node *get_clk_provider_node(struct device *dev) > * @get: callback for decoding clk_hw > * @data: context pointer for @get callback > * > - * Registers clock provider for given device's node. If the device has no DT > - * node or if the device node lacks of clock provider information (#clock-cells) > - * then the parent device's node is scanned for this information. If parent node > - * has the #clock-cells then it is used in registration. Provider is > - * automatically released at device exit. > + * Registers clock provider for given device's node. If the device node lacks > + * of clock provider information (#clock-cells) then the parent device's node is > + * scanned for this information. If parent node has the #clock-cells then it is > + * used in registration. Provider is automatically released at device exit. > * > * Return: 0 on success or an errno on failure. > */ > @@ -4650,6 +4649,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > struct device_node **ptr, *np; > int ret; > > + if (!dev->of_node) > + return -ENODEV; > + Based on the other discussions, for now, just return 0. The error might cause other issues in other drivers. We can clean this up later. -Saravana > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > GFP_KERNEL); > if (!ptr) > -- > 2.25.1 >
Hi Saravana, Tudor, On Fri, 2021-04-23 at 10:24 -0700, Saravana Kannan wrote: > On Fri, Apr 23, 2021 at 10:14 AM 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. > > > > devm_of_clk_add_hw_provider() should not register the provider with > > a NULL dev->of_node, as there is no of_node. Apart of the NULL pointer > > dereference that will result when calling fwnode_dev_initialized() in > > of_clk_add_hw_provider(), another problem is that when two drivers calling > > of_clk_add_hw_provider() with np = NULL, their unregistration order is not > > guaranteed to be correct. Avoid all the problems and just return -ENODEV > > when the callers of devm_of_clk_add_hw_provider() use a NULL dev->of_node, > > which seems the natural way to do. > > > > 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> > > --- > > drivers/clk/clk.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index e2ec1b745243..8b5077cc5e67 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -4634,11 +4634,10 @@ static struct device_node *get_clk_provider_node(struct device *dev) > > * @get: callback for decoding clk_hw > > * @data: context pointer for @get callback > > * > > - * Registers clock provider for given device's node. If the device has no DT > > - * node or if the device node lacks of clock provider information (#clock-cells) > > - * then the parent device's node is scanned for this information. If parent node > > - * has the #clock-cells then it is used in registration. Provider is > > - * automatically released at device exit. > > + * Registers clock provider for given device's node. If the device node lacks > > + * of clock provider information (#clock-cells) then the parent device's node is > > + * scanned for this information. If parent node has the #clock-cells then it is > > + * used in registration. Provider is automatically released at device exit. > > * > > * Return: 0 on success or an errno on failure. > > */ > > @@ -4650,6 +4649,9 @@ int devm_of_clk_add_hw_provider(struct device *dev, > > struct device_node **ptr, *np; > > int ret; > > > > + if (!dev->of_node) > > + return -ENODEV; > > + > > Based on the other discussions, for now, just return 0. The error > might cause other issues in other drivers. We can clean this up later. +1, Let's return 0 and do nothing skip the logic in the driver. Now, from what I read in devm_of_clk_add_hw_provider(), there is a use case for entering with '!dev->of_node'. See get_clk_provider_node()'s usage. So I think we should only bail if that function fails to provide a device_node. Regards, Nicolas
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) { >
This fixes a NULL pointer dereference that was revealed by commit 6579c8d97ad7 ("clk: Mark fwnodes when their clock provider is added") I chose to just return -ENODEV when a driver calls devm_of_clk_add_hw_provider() with a NULL dev->of_node, because it seems natural to return a failure when one asks for an _of_ function to do something, but by providing with a NULL of_node. Plus, it avoids some waste of memory, as the call to devres_alloc() will not take place. The opinions on how to handle this NULL pointer dereference were split, either by returning an error when one calls devm_of_clk_add_hw_provider() with a NULL dev->of_node, as I did, or by return 0 in of_clk_add_hw_provider() when np == NULL and return 0 in of_clk_del_provider() when np == NULL. Let me know if you prefer the second approach. Compile tested only. Tudor Ambarus (2): clk: Do not register provider with a NULL dev->of_node clk: bcm: rpi: Do not call devm_of_clk_add_hw_provider with a NULL dev->of_node drivers/clk/bcm/clk-raspberrypi.c | 10 ++++++---- drivers/clk/clk.c | 12 +++++++----- 2 files changed, 13 insertions(+), 9 deletions(-)