linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pinctrl:sunplus: Add check for kmalloc
@ 2023-05-23 10:11 Wells Lu
  2023-05-23 16:42 ` andy.shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Wells Lu @ 2023-05-23 10:11 UTC (permalink / raw)
  To: linus.walleij, linux-gpio, linux-kernel; +Cc: wells.lu, Wells Lu

Fix Smatch static checker warning:
potential null dereference 'configs'. (kmalloc returns null)

Fixes: aa74c44be19c ("pinctrl: Add driver for Sunplus SP7021")
Signed-off-by: Wells Lu <wellslutw@gmail.com>
---
 drivers/pinctrl/sunplus/sppctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c
index 6bbbab3a6..f2dcc68ee 100644
--- a/drivers/pinctrl/sunplus/sppctl.c
+++ b/drivers/pinctrl/sunplus/sppctl.c
@@ -883,6 +883,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
 			(*map)[i].data.configs.num_configs = 1;
 			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
 			configs = kmalloc(sizeof(*configs), GFP_KERNEL);
+			if (!configs)
+				return -ENOMEM;
 			*configs = FIELD_GET(GENMASK(7, 0), dt_pin);
 			(*map)[i].data.configs.configs = configs;
 
@@ -896,6 +898,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
 			(*map)[i].data.configs.num_configs = 1;
 			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
 			configs = kmalloc(sizeof(*configs), GFP_KERNEL);
+			if (!configs)
+				return -ENOMEM;
 			*configs = SPPCTL_IOP_CONFIGS;
 			(*map)[i].data.configs.configs = configs;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 10:11 [PATCH] pinctrl:sunplus: Add check for kmalloc Wells Lu
@ 2023-05-23 16:42 ` andy.shevchenko
  2023-05-23 17:39   ` Wells Lu 呂芳騰
  0 siblings, 1 reply; 12+ messages in thread
From: andy.shevchenko @ 2023-05-23 16:42 UTC (permalink / raw)
  To: Wells Lu; +Cc: linus.walleij, linux-gpio, linux-kernel, wells.lu

Tue, May 23, 2023 at 06:11:28PM +0800, 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.

...

>  			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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 16:42 ` andy.shevchenko
@ 2023-05-23 17:39   ` Wells Lu 呂芳騰
  2023-05-23 19:37     ` andy.shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Wells Lu 呂芳騰 @ 2023-05-23 17:39 UTC (permalink / raw)
  To: andy.shevchenko, Wells Lu; +Cc: linus.walleij, linux-gpio, linux-kernel

> > 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?


> ...
> 
> >  			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.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 


Best regards,
Wells Lu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 17:39   ` Wells Lu 呂芳騰
@ 2023-05-23 19:37     ` andy.shevchenko
  2023-05-23 20:05       ` Christophe JAILLET
  0 siblings, 1 reply; 12+ messages in thread
From: andy.shevchenko @ 2023-05-23 19:37 UTC (permalink / raw)
  To: Wells Lu 呂芳騰
  Cc: andy.shevchenko, Wells Lu, linus.walleij, linux-gpio, linux-kernel

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.

^^^

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 19:37     ` andy.shevchenko
@ 2023-05-23 20:05       ` Christophe JAILLET
  2023-05-25  3:22         ` Wells Lu 呂芳騰
  2023-05-25 19:19         ` Dan Carpenter
  0 siblings, 2 replies; 12+ messages in thread
From: Christophe JAILLET @ 2023-05-23 20:05 UTC (permalink / raw)
  To: andy.shevchenko, Wells Lu 呂芳騰
  Cc: Wells Lu, linus.walleij, linux-gpio, linux-kernel

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#L978)

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 20:05       ` Christophe JAILLET
@ 2023-05-25  3:22         ` Wells Lu 呂芳騰
  2023-05-25 18:37           ` Christophe JAILLET
  2023-05-25 19:19         ` Dan Carpenter
  1 sibling, 1 reply; 12+ messages in thread
From: Wells Lu 呂芳騰 @ 2023-05-25  3:22 UTC (permalink / raw)
  To: Christophe JAILLET, andy.shevchenko
  Cc: Wells Lu, linus.walleij, linux-gpio, linux-kernel

> 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.


Best regards,
Wells Lu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-25  3:22         ` Wells Lu 呂芳騰
@ 2023-05-25 18:37           ` Christophe JAILLET
  2023-05-25 19:42             ` Dan Carpenter
  2023-05-26  2:04             ` Wells Lu 呂芳騰
  0 siblings, 2 replies; 12+ messages in thread
From: Christophe JAILLET @ 2023-05-25 18:37 UTC (permalink / raw)
  To: Wells Lu 呂芳騰, andy.shevchenko, Dan Carpenter
  Cc: Wells Lu, linus.walleij, linux-gpio, linux-kernel, Kernel Janitors

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-23 20:05       ` Christophe JAILLET
  2023-05-25  3:22         ` Wells Lu 呂芳騰
@ 2023-05-25 19:19         ` Dan Carpenter
  2023-05-25 20:00           ` Christophe JAILLET
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2023-05-25 19:19 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: andy.shevchenko, Wells Lu 呂芳騰,
	Wells Lu, linus.walleij, linux-gpio, linux-kernel

On Tue, May 23, 2023 at 10:05:49PM +0200, Christophe JAILLET wrote:
> 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)

Thanks for this call tree, I don't have this file enabled in my build
so it's not easy for me to find how sppctl_dt_node_to_map() was called.

drivers/pinctrl/devicetree.c
   160                  dev_err(p->dev, "pctldev %s doesn't support DT\n",
   161                          dev_name(pctldev->dev));
   162                  return -ENODEV;
   163          }
   164          ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
                                                              ^^^^
"map" isn't stored anywhere so it will be leaked.  I guess kmemleak
already figured this out.

   165          if (ret < 0)
   166                  return ret;
   167          else if (num_maps == 0) {
   168                  /*

> 
> 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#L978)
> 
> 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.
> 

Yes.  There are some missing calls to of_node_put(parent); in
sppctl_dt_node_to_map().

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-25 18:37           ` Christophe JAILLET
@ 2023-05-25 19:42             ` Dan Carpenter
  2023-05-26  2:04             ` Wells Lu 呂芳騰
  1 sibling, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2023-05-25 19:42 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Wells Lu 呂芳騰,
	andy.shevchenko, Wells Lu, linus.walleij, linux-gpio,
	linux-kernel, Kernel Janitors

On Thu, May 25, 2023 at 08:37:36PM +0200, Christophe JAILLET wrote:
> 
> 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.

Smatch doesn't really complain about memory leaks.  I wrote that check
13 years ago and focused more on avoiding false positives instead of
being thorough.  It turns out that avoiding false positives is
achievable but pretty useless.

Checking for the of_get_parent() leaks is probably easier.  I could just
add it to check_unwind.c or create something custom.  The heuristic for
the custom check would be to print a warning if the success path
releases it but the others don't.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-25 19:19         ` Dan Carpenter
@ 2023-05-25 20:00           ` Christophe JAILLET
  0 siblings, 0 replies; 12+ messages in thread
From: Christophe JAILLET @ 2023-05-25 20:00 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: andy.shevchenko, Wells Lu 呂芳騰,
	Wells Lu, linus.walleij, linux-gpio, linux-kernel

Le 25/05/2023 à 21:19, Dan Carpenter a écrit :
> On Tue, May 23, 2023 at 10:05:49PM +0200, Christophe JAILLET wrote:
>> 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)
> 
> Thanks for this call tree, I don't have this file enabled in my build
> so it's not easy for me to find how sppctl_dt_node_to_map() was called.
> 
> drivers/pinctrl/devicetree.c
>     160                  dev_err(p->dev, "pctldev %s doesn't support DT\n",
>     161                          dev_name(pctldev->dev));
>     162                  return -ENODEV;
>     163          }
>     164          ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
>                                                                ^^^^
> "map" isn't stored anywhere so it will be leaked.  I guess kmemleak
> already figured this out.
> 
>     165          if (ret < 0)
>     166                  return ret;
>     167          else if (num_maps == 0) {
>     168                  /*
> 

Hi, thanks Dan for sharing your PoV on this.

CJ


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-25 18:37           ` Christophe JAILLET
  2023-05-25 19:42             ` Dan Carpenter
@ 2023-05-26  2:04             ` Wells Lu 呂芳騰
  2023-05-26  5:19               ` Christophe JAILLET
  1 sibling, 1 reply; 12+ messages in thread
From: Wells Lu 呂芳騰 @ 2023-05-26  2:04 UTC (permalink / raw)
  To: Christophe JAILLET, andy.shevchenko, Dan Carpenter
  Cc: Wells Lu, linus.walleij, linux-gpio, linux-kernel, Kernel Janitors

Hi,

> 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/devi
> >> cetree.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/sunp
> >> lus/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/pinc
> >> trl-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

I forced sppctl_dt_node_to_map() to always return -ENOMEM when it configs GPIO pins. 
Here is the report of kmemleak:

~ # echo scan > /sys/kernel/debug/kmemleak
[    9.136464] kmemleak: 2 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
~ # 
~ # cat /sys/kernel/debug/kmemleak
unreferenced object 0xc2852e00 (size 64):
  comm "kworker/u8:3", pid 36, jiffies 4294937312 (age 13.610s)
  hex dump (first 32 bytes):
    00 00 00 00 14 7c ff df 03 00 00 00 00 00 00 00  .....|..........
    c4 8f 3a c0 00 00 00 00 01 00 00 00 00 00 00 00  ..:.............
  backtrace:
    [<(ptrval)>] slab_post_alloc_hook.constprop.16+0x4b/0x52
    [<(ptrval)>] __kmem_cache_alloc_node+0x57/0x78
    [<(ptrval)>] __kmalloc+0x33/0x3a
    [<(ptrval)>] sppctl_dt_node_to_map+0x77/0x3cc
    [<(ptrval)>] pinctrl_dt_to_map+0x16f/0x20e
    [<(ptrval)>] create_pinctrl+0x3d/0x224
    [<(ptrval)>] devm_pinctrl_get+0x23/0x50
    [<(ptrval)>] pinctrl_bind_pins+0x31/0x190
    [<(ptrval)>] really_probe+0x89/0x23e
    [<(ptrval)>] __driver_probe_device+0x131/0x164
    [<(ptrval)>] driver_probe_device+0x2d/0x88
    [<(ptrval)>] __device_attach_driver+0x8d/0xa4
    [<(ptrval)>] bus_for_each_drv+0x63/0x72
    [<(ptrval)>] __device_attach+0x89/0xe2
    [<(ptrval)>] bus_probe_device+0x1f/0x5a
    [<(ptrval)>] deferred_probe_work_func+0x69/0x88
unreferenced object 0xc2852dc0 (size 64):
  comm "kworker/u8:3", pid 36, jiffies 4294937312 (age 13.610s)
  hex dump (first 32 bytes):
    01 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 54 44 00 00 00 00 00 02  ........TD......
  backtrace:
    [<(ptrval)>] slab_post_alloc_hook.constprop.16+0x4b/0x52
    [<(ptrval)>] __kmem_cache_alloc_node+0x57/0x78
    [<(ptrval)>] kmalloc_trace+0x11/0x16
    [<(ptrval)>] sppctl_dt_node_to_map+0x14b/0x3cc
    [<(ptrval)>] pinctrl_dt_to_map+0x16f/0x20e
    [<(ptrval)>] create_pinctrl+0x3d/0x224
    [<(ptrval)>] devm_pinctrl_get+0x23/0x50
    [<(ptrval)>] pinctrl_bind_pins+0x31/0x190
    [<(ptrval)>] really_probe+0x89/0x23e
    [<(ptrval)>] __driver_probe_device+0x131/0x164
    [<(ptrval)>] driver_probe_device+0x2d/0x88
    [<(ptrval)>] __device_attach_driver+0x8d/0xa4
    [<(ptrval)>] bus_for_each_drv+0x63/0x72
    [<(ptrval)>] __device_attach+0x89/0xe2
    [<(ptrval)>] bus_probe_device+0x1f/0x5a
    [<(ptrval)>] deferred_probe_work_func+0x69/0x88
~ #


Best regards,
Wells Lu


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: RE: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc
  2023-05-26  2:04             ` Wells Lu 呂芳騰
@ 2023-05-26  5:19               ` Christophe JAILLET
  0 siblings, 0 replies; 12+ messages in thread
From: Christophe JAILLET @ 2023-05-26  5:19 UTC (permalink / raw)
  To: Wells Lu 呂芳騰, andy.shevchenko, Dan Carpenter
  Cc: Wells Lu, linus.walleij, linux-gpio, linux-kernel, Kernel Janitors

Le 26/05/2023 à 04:04, Wells Lu 呂芳騰 a écrit :
> Hi,
> 
> I forced sppctl_dt_node_to_map() to always return -ENOMEM when it configs GPIO pins.
> Here is the report of kmemleak:
> 

Nice, thanks.

CJ


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-05-26  5:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-23 10:11 [PATCH] pinctrl:sunplus: Add check for kmalloc Wells Lu
2023-05-23 16:42 ` andy.shevchenko
2023-05-23 17:39   ` Wells Lu 呂芳騰
2023-05-23 19:37     ` andy.shevchenko
2023-05-23 20:05       ` Christophe JAILLET
2023-05-25  3:22         ` Wells Lu 呂芳騰
2023-05-25 18:37           ` Christophe JAILLET
2023-05-25 19:42             ` Dan Carpenter
2023-05-26  2:04             ` Wells Lu 呂芳騰
2023-05-26  5:19               ` Christophe JAILLET
2023-05-25 19:19         ` Dan Carpenter
2023-05-25 20:00           ` Christophe JAILLET

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).