Messages in this thread | | | Date | Thu, 25 May 2023 20:37:36 +0200 | Subject | Re: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc | From | Christophe JAILLET <> |
| |
Le 25/05/2023 à 05:22, Wells Lu 呂芳騰 a écrit : >> Le 23/05/2023 à 21:37, andy.shevchenko@gmail.com a écrit : >>> Tue, May 23, 2023 at 05:39:51PM +0000, Wells Lu 呂芳騰 kirjoitti: >>>>>> Fix Smatch static checker warning: >>>>>> potential null dereference 'configs'. (kmalloc returns null) >>> >>> ... >>> >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); >>>>>> + if (!configs) >>>>> >>>>>> + return -ENOMEM; >>>>> >>>>> "Fixing" by adding a memory leak is not probably a good approach. >>>> >>>> Do you mean I need to free all memory which are allocated in this >>>> subroutine before return -ENOMEM? >>> >>> This is my understanding of the code. But as I said... (see below) >>> >>> ... >>> >>>>>> configs = kmalloc(sizeof(*configs), GFP_KERNEL); >>>>>> + if (!configs) >>>>>> + return -ENOMEM; >>>>> >>>>> Ditto. >>> >>> ... >>> >>>>> It might be that I'm mistaken. In this case please add an >>>>> explanation why in the commit message. >>> >>> ^^^ >>> >> >> Hmmm, not so sure. >> >> Should be looked at more carefully, but >> dt_to_map_one_config (in /drivers/pinctrl/devicetree.c) >> .dt_node_to_map >> --> sppctl_dt_node_to_map >> >> Should dt_to_map_one_config() fail, pinctrl_dt_free_maps() is called (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/devicetree.c#L281) >> >> pinctrl_dt_free_maps() calls dt_free_map(), which calls .dt_free_map, so >> pinctrl_utils_free_map() >> (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/sunplus/sppctl.c#L97 >> 8) >> >> Finally the needed kfree seem to be called from here. >> (see >> https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/pinctrl/pinctrl-utils.c#L119 >> ) >> >> >> *This should obviously be double checked*, but looks safe to me. >> >> >> BUT, in the same function, the of_get_parent() should be undone in case >> of error, as done at the end of the function, in the normal path. >> This one seems to be missing, should a memory allocation error occur. >> >> >> Just my 2c, >> >> CJ > > Thank you for your comments. > > From the report of kmemleak, returning -ENOMEM directly causes memory leak. We > need to free any memory allocated in this subroutine before returning -ENOMEM. > > I'll send a new patch that will free the allocated memory and call of_node_put() > when an error happens.
Hi, (adding Dan in copy because the initial report is related to smatch)
I don't use kmemleak, but could you share some input about its report?
I've not rechecked my analysis, but it looked promising. Maybe Dan could also give a look at it and confirm your finding, or dig further with smatch to make sure that its static analysis was complete enough.
CJ
> > > Best regards, > Wells Lu
| |