* [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' @ 2020-09-22 21:05 Christophe JAILLET 2020-09-23 13:27 ` Dan Murphy ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Christophe JAILLET @ 2020-09-22 21:05 UTC (permalink / raw) To: pavel, dmurphy, jacek.anaszewski Cc: linux-leds, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET @ 2020-09-23 13:27 ` Dan Murphy 2020-09-23 13:35 ` Dan Carpenter 2020-11-25 11:01 ` Pavel Machek 2 siblings, 0 replies; 9+ messages in thread From: Dan Murphy @ 2020-09-23 13:27 UTC (permalink / raw) To: Christophe JAILLET, pavel, jacek.anaszewski Cc: linux-leds, linux-kernel, kernel-janitors 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET 2020-09-23 13:27 ` Dan Murphy @ 2020-09-23 13:35 ` Dan Carpenter 2020-09-23 18:49 ` Christophe JAILLET 2020-11-25 11:01 ` Pavel Machek 2 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2020-09-23 13:35 UTC (permalink / raw) To: Christophe JAILLET, Heikki Krogerus Cc: pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-23 13:35 ` Dan Carpenter @ 2020-09-23 18:49 ` Christophe JAILLET 2020-09-24 6:49 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Christophe JAILLET @ 2020-09-23 18:49 UTC (permalink / raw) To: Dan Carpenter, Heikki Krogerus Cc: pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-23 18:49 ` Christophe JAILLET @ 2020-09-24 6:49 ` Dan Carpenter 2020-09-28 11:53 ` Heikki Krogerus 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2020-09-24 6:49 UTC (permalink / raw) To: Christophe JAILLET Cc: Heikki Krogerus, pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-24 6:49 ` Dan Carpenter @ 2020-09-28 11:53 ` Heikki Krogerus 2020-11-25 10:46 ` Pavel Machek 0 siblings, 1 reply; 9+ messages in thread From: Heikki Krogerus @ 2020-09-28 11:53 UTC (permalink / raw) To: Dan Carpenter Cc: Christophe JAILLET, pavel, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 1453 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-28 11:53 ` Heikki Krogerus @ 2020-11-25 10:46 ` Pavel Machek 2020-11-25 12:16 ` Heikki Krogerus 0 siblings, 1 reply; 9+ messages in thread From: Pavel Machek @ 2020-11-25 10:46 UTC (permalink / raw) To: Heikki Krogerus Cc: Dan Carpenter, Christophe JAILLET, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-11-25 10:46 ` Pavel Machek @ 2020-11-25 12:16 ` Heikki Krogerus 0 siblings, 0 replies; 9+ messages in thread From: Heikki Krogerus @ 2020-11-25 12:16 UTC (permalink / raw) To: Pavel Machek Cc: Dan Carpenter, Christophe JAILLET, dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' 2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET 2020-09-23 13:27 ` Dan Murphy 2020-09-23 13:35 ` Dan Carpenter @ 2020-11-25 11:01 ` Pavel Machek 2 siblings, 0 replies; 9+ messages in thread From: Pavel Machek @ 2020-11-25 11:01 UTC (permalink / raw) To: Christophe JAILLET Cc: dmurphy, jacek.anaszewski, linux-leds, linux-kernel, kernel-janitors [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-25 12:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 21:05 [PATCH] leds: lp50xx: Fix an error handling path in 'lp50xx_probe_dt()' Christophe JAILLET 2020-09-23 13:27 ` Dan Murphy 2020-09-23 13:35 ` Dan Carpenter 2020-09-23 18:49 ` Christophe JAILLET 2020-09-24 6:49 ` Dan Carpenter 2020-09-28 11:53 ` Heikki Krogerus 2020-11-25 10:46 ` Pavel Machek 2020-11-25 12:16 ` Heikki Krogerus 2020-11-25 11:01 ` Pavel Machek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).