Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()'
From: Dan Carpenter
Date: Wed Sep 23 2020 - 09:36:36 EST
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@xxxxxxxxxx>
> ---
> 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