[PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Christophe JAILLET]

In case of memory allocation failure, we must release some resources as
done in all other error handling paths of the function.

'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
is called when we break out of a 'device_for_each_child_node' loop.

Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/leds/leds-lp50xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 47144a37cb94..8178782f2a8a 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 		 */
 		mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
 					   sizeof(*mc_led_info), GFP_KERNEL);
-		if (!mc_led_info)
-			return -ENOMEM;
+		if (!mc_led_info) {
+			ret = -ENOMEM;
+			goto child_out;
+		}
 
 		fwnode_for_each_child_node(child, led_node) {
 			ret = fwnode_property_read_u32(led_node, "color",
-- 
2.25.1

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Dan Murphy]

Christophe

On 9/22/20 4:05 PM, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
>
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
>
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/leds/leds-lp50xx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 47144a37cb94..8178782f2a8a 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>   		 */
>   		mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
>   					   sizeof(*mc_led_info), GFP_KERNEL);
> -		if (!mc_led_info)
> -			return -ENOMEM;
> +		if (!mc_led_info) {
> +			ret = -ENOMEM;
> +			goto child_out;
> +		}
>   
>   		fwnode_for_each_child_node(child, led_node) {
>   			ret = fwnode_property_read_u32(led_node, "color",

Thanks for the patch

Acked-by: Dan Murphy <dmurphy@ti.com>

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Dan Carpenter]

I've added Heikki Krogerus to the CC list because my question is mostly
about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
the firmware node framework").

I have been trying to teach Smatch to understand reference counting so
it can discover these kinds of bugs automatically.

I don't know how software_node_get_next_child() can work when it doesn't
call kobject_get().  This sort of bug would have been caught in testing
because it affects the success path so I must be reading the code wrong.

drivers/leds/leds-lp50xx.c
   444  static int lp50xx_probe_dt(struct lp50xx *priv)
   445  {
   446          struct fwnode_handle *child = NULL;
   447          struct fwnode_handle *led_node = NULL;
   448          struct led_init_data init_data = {};
   449          struct led_classdev *led_cdev;
   450          struct mc_subled *mc_led_info;
   451          struct lp50xx_led *led;
   452          int ret = -EINVAL;
   453          int num_colors;
   454          u32 color_id;
   455          int i = 0;
   456  
   457          priv->enable_gpio = devm_gpiod_get_optional(priv->dev, "enable", GPIOD_OUT_LOW);
   458          if (IS_ERR(priv->enable_gpio)) {
   459                  ret = PTR_ERR(priv->enable_gpio);
   460                  dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
   461                          ret);
   462                  return ret;
   463          }
   464  
   465          priv->regulator = devm_regulator_get(priv->dev, "vled");
   466          if (IS_ERR(priv->regulator))
   467                  priv->regulator = NULL;
   468  
   469          device_for_each_child_node(priv->dev, child) {
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
This iterator is implemented by two possible function pointers.  Smatch
understands of_fwnode_get_next_child_node() and that it takes a
reference.  The software_node_get_next_child() function does not take
a kobject reference.

   470                  led = &priv->leds[i];
   471                  ret = fwnode_property_count_u32(child, "reg");
   472                  if (ret < 0) {
   473                          dev_err(&priv->client->dev, "reg property is invalid\n");
   474                          goto child_out;
   475                  }
   476  
   477                  ret = lp50xx_probe_leds(child, priv, led, ret);
   478                  if (ret)
   479                          goto child_out;
   480  
   481                  init_data.fwnode = child;
   482                  num_colors = 0;
   483  
   484                  /*
   485                   * There are only 3 LEDs per module otherwise they should be
   486                   * banked which also is presented as 3 LEDs.
   487                   */
   488                  mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
   489                                             sizeof(*mc_led_info), GFP_KERNEL);
   490                  if (!mc_led_info)
   491                          return -ENOMEM;
   492  
   493                  fwnode_for_each_child_node(child, led_node) {
   494                          ret = fwnode_property_read_u32(led_node, "color",
   495                                                         &color_id);
   496                          if (ret) {
   497                                  dev_err(priv->dev, "Cannot read color\n");
   498                                  goto child_out;
   499                          }
   500  
   501                          mc_led_info[num_colors].color_index = color_id;
   502                          num_colors++;
   503                  }
   504  
   505                  led->priv = priv;
   506                  led->mc_cdev.num_colors = num_colors;
   507                  led->mc_cdev.subled_info = mc_led_info;
   508                  led_cdev = &led->mc_cdev.led_cdev;
   509                  led_cdev->brightness_set_blocking = lp50xx_brightness_set;
   510  
   511                  fwnode_property_read_string(child, "linux,default-trigger",
   512                                              &led_cdev->default_trigger);
   513  
   514                  ret = devm_led_classdev_multicolor_register_ext(&priv->client->dev,
   515                                                         &led->mc_cdev,
   516                                                         &init_data);
   517                  if (ret) {
   518                          dev_err(&priv->client->dev, "led register err: %d\n",
   519                                  ret);
   520                          goto child_out;
   521                  }
   522                  i++;
   523                  fwnode_handle_put(child);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^
This will call software_node_put() which calls kobject_put().

   524          }
   525  
   526          return 0;
   527  
   528  child_out:
   529          fwnode_handle_put(child);
                ^^^^^^^^^^^^^^^^^^^^^^^^
Same here.

   530          return ret;
   531  }

regards,
dan carpenter

On Tue, Sep 22, 2020 at 11:05:15PM +0200, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
> 
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
> 
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/leds/leds-lp50xx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 47144a37cb94..8178782f2a8a 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -487,8 +487,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>  		 */
>  		mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
>  					   sizeof(*mc_led_info), GFP_KERNEL);
> -		if (!mc_led_info)
> -			return -ENOMEM;
> +		if (!mc_led_info) {
> +			ret = -ENOMEM;
> +			goto child_out;
> +		}
>  
>  		fwnode_for_each_child_node(child, led_node) {
>  			ret = fwnode_property_read_u32(led_node, "color",
> -- 
> 2.25.1

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Christophe JAILLET]

Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> I've added Heikki Krogerus to the CC list because my question is mostly
> about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> the firmware node framework").
> 
> I have been trying to teach Smatch to understand reference counting so
> it can discover these kinds of bugs automatically.
> 
> I don't know how software_node_get_next_child() can work when it doesn't
> call kobject_get().  This sort of bug would have been caught in testing
> because it affects the success path so I must be reading the code wrong.
> 

I had the same reading of the code and thought that I was missing 
something somewhere.

There is the same question about 'acpi_get_next_subnode' which is also a 
'.get_next_child_node' function, without any ref counting, if I'm correct.

CJ

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Dan Carpenter]

On Wed, Sep 23, 2020 at 08:49:56PM +0200, Christophe JAILLET wrote:
> Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> > I've added Heikki Krogerus to the CC list because my question is mostly
> > about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> > the firmware node framework").
> > 
> > I have been trying to teach Smatch to understand reference counting so
> > it can discover these kinds of bugs automatically.
> > 
> > I don't know how software_node_get_next_child() can work when it doesn't
> > call kobject_get().  This sort of bug would have been caught in testing
> > because it affects the success path so I must be reading the code wrong.
> > 
> 
> I had the same reading of the code and thought that I was missing something
> somewhere.
> 
> There is the same question about 'acpi_get_next_subnode' which is also a
> '.get_next_child_node' function, without any ref counting, if I'm correct.
> 

Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
stuff so it's not a problem.  (Presumably there is some other sort of
refcounting policy there).

regards,
dan carpenter

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

On Thu, Sep 24, 2020 at 09:49:32AM +0300, Dan Carpenter wrote:
> On Wed, Sep 23, 2020 at 08:49:56PM +0200, Christophe JAILLET wrote:
> > Le 23/09/2020 à 15:35, Dan Carpenter a écrit :
> > > I've added Heikki Krogerus to the CC list because my question is mostly
> > > about commit 59abd83672f7 ("drivers: base: Introducing software nodes to
> > > the firmware node framework").
> > > 
> > > I have been trying to teach Smatch to understand reference counting so
> > > it can discover these kinds of bugs automatically.
> > > 
> > > I don't know how software_node_get_next_child() can work when it doesn't
> > > call kobject_get().  This sort of bug would have been caught in testing
> > > because it affects the success path so I must be reading the code wrong.
> > > 
> > 
> > I had the same reading of the code and thought that I was missing something
> > somewhere.
> > 
> > There is the same question about 'acpi_get_next_subnode' which is also a
> > '.get_next_child_node' function, without any ref counting, if I'm correct.
> > 
> 
> Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> stuff so it's not a problem.  (Presumably there is some other sort of
> refcounting policy there).

OK, so I guess we need to make software_node_get_next_child()
mimic the behaviour of of_get_next_available_child(), and not
acpi_get_next_subnode(). Does the attached patch work?


thanks,

-- 
heikki

[-- Attachment #2: 0001-software-nodes-Handle-the-refcounting-also-in-softwa.patch --]
[-- Type: text/plain, Size: 1765 bytes --]

From 9b5744450d07b1e6e32e441785b9b69d7e54a7b1 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 28 Sep 2020 14:38:09 +0300
Subject: [PATCH] software nodes: Handle the refcounting also in
 software_node_get_next_child()

Incrementing the reference count of the node that is
returned in software_node_get_next_child(), and decrementing
the reference count of the previous node.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785bc..adbaafab3887b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -439,18 +439,26 @@ static struct fwnode_handle *
 software_node_get_next_child(const struct fwnode_handle *fwnode,
 			     struct fwnode_handle *child)
 {
-	struct swnode *p = to_swnode(fwnode);
-	struct swnode *c = to_swnode(child);
+	struct swnode *parent = to_swnode(fwnode);
+	struct swnode *prev = to_swnode(child);
+	struct swnode *next;
 
-	if (!p || list_empty(&p->children) ||
-	    (c && list_is_last(&c->entry, &p->children)))
+	if (!parent || list_empty(&parent->children))
 		return NULL;
 
-	if (c)
-		c = list_next_entry(c, entry);
-	else
-		c = list_first_entry(&p->children, struct swnode, entry);
-	return &c->fwnode;
+	if (prev && list_is_last(&prev->entry, &parent->children)) {
+		kobject_put(&prev->kobj);
+		return NULL;
+	}
+
+	if (prev) {
+		next = list_next_entry(prev, entry);
+		kobject_put(&prev->kobj);
+	} else {
+		next = list_first_entry(&parent->children, struct swnode, entry);
+	}
+
+	return fwnode_handle_get(&next->fwnode);
 }
 
 static struct fwnode_handle *
-- 
2.28.0

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

Hi!

> > > > I have been trying to teach Smatch to understand reference counting so
> > > > it can discover these kinds of bugs automatically.
> > > > 
> > > > I don't know how software_node_get_next_child() can work when it doesn't
> > > > call kobject_get().  This sort of bug would have been caught in testing
> > > > because it affects the success path so I must be reading the code wrong.
> > > > 
> > > 
> > > I had the same reading of the code and thought that I was missing something
> > > somewhere.
> > > 
> > > There is the same question about 'acpi_get_next_subnode' which is also a
> > > '.get_next_child_node' function, without any ref counting, if I'm correct.
> > > 
> > 
> > Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> > stuff so it's not a problem.  (Presumably there is some other sort of
> > refcounting policy there).
> 
> OK, so I guess we need to make software_node_get_next_child()
> mimic the behaviour of of_get_next_available_child(), and not
> acpi_get_next_subnode(). Does the attached patch work?

Does not sound unreasonable. Did it get solved, somehow?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Tue 2020-09-22 23:05:15, Christophe JAILLET wrote:
> In case of memory allocation failure, we must release some resources as
> done in all other error handling paths of the function.
> 
> 'goto child_out' instead of a direct return so that 'fwnode_handle_put()'
> is called when we break out of a 'device_for_each_child_node' loop.
> 
> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks, applied.

Best regards,
								Pavel
								
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'

[Heikki Krogerus]

On Wed, Nov 25, 2020 at 11:46:29AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > > I have been trying to teach Smatch to understand reference counting so
> > > > > it can discover these kinds of bugs automatically.
> > > > > 
> > > > > I don't know how software_node_get_next_child() can work when it doesn't
> > > > > call kobject_get().  This sort of bug would have been caught in testing
> > > > > because it affects the success path so I must be reading the code wrong.
> > > > > 
> > > > 
> > > > I had the same reading of the code and thought that I was missing something
> > > > somewhere.
> > > > 
> > > > There is the same question about 'acpi_get_next_subnode' which is also a
> > > > '.get_next_child_node' function, without any ref counting, if I'm correct.
> > > > 
> > > 
> > > Yeah, but there aren't any ->get/put() ops for the acpi_get_next_subnode()
> > > stuff so it's not a problem.  (Presumably there is some other sort of
> > > refcounting policy there).
> > 
> > OK, so I guess we need to make software_node_get_next_child()
> > mimic the behaviour of of_get_next_available_child(), and not
> > acpi_get_next_subnode(). Does the attached patch work?
> 
> Does not sound unreasonable. Did it get solved, somehow?

Has anybody tested my patch?

thanks,

-- 
heikki