Hi, > Le 25/05/2023 à 05:22, Wells Lu 呂芳騰 a écrit : > >> Le 23/05/2023 à 21:37, andy.shevchenko@xxxxxxxxx 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
