| Message ID | 20200922210515.385099-1-christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' | expand |
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>
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
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
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
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,
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
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
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
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",
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(-)