[PATCH] pinctrl:sunplus: Add check for kmalloc

[PATCH] pinctrl:sunplus: Add check for kmalloc

[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

Re: [PATCH] pinctrl:sunplus: Add check for kmalloc

[andy.shevchenko]

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

RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Wells Lu 呂芳騰]

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

Re: [PATCH] pinctrl:sunplus: Add check for kmalloc

[andy.shevchenko]

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

Re: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Christophe JAILLET]

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

RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Wells Lu 呂芳騰]

> 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

Re: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[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

Re: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Dan Carpenter]

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

RE: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Wells Lu 呂芳騰]

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

Re: RE: RE: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Christophe JAILLET]

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

Re: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Dan Carpenter]

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

Re: [PATCH] pinctrl:sunplus: Add check for kmalloc

[Christophe JAILLET]

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