[PATCH] pinctrl: single: Fix memleak in pcs_dt_node_to_map

[guomengqi (A)]

在 2023/7/6 12:07, Tony Lindgren 写道:
> Hi,
>
> * guomengqi (A) <guomengqi3 at huawei.com> [230706 03:21]:
>> 在 2023/7/4 17:18, Linus Walleij 写道:
>>> On Mon, Jul 3, 2023 at 10:24 AM Guo Mengqi <guomengqi3 at huawei.com> wrote:
>>>
>>>> In a reliability test which repeatedly load and remove a module,
>>>> I found some kmalloc-256 memory leaks in pinctrl-single.
>>>>
>>>> pcs_dt_node_to_map() will recognize a dt_node and
>>>> make a mapping for it. Along the way some pinctrl functions and groups
>>>> are registered in pinctrl-single controller. These functions/groups are
>>>> registered once and not removed during the system lifetime.
>>>>
>>>> When the client module loads again, pcs_dt_node_to_map() fail to consider
>>>> this situation, create the same set of resources, and does not release or
>>>> use them.
>>>>
>>>> To fix this, add a check at the start of pcs_parse_one_pinctrl_entry/
>>>> pcs_parse_bits_in_pinctrl_entry. If the target is found,
>>>> then all the resource allocation and parsing work can be skipped,
>>>> just set the mapping with existing function/group information.
>>>>
>>>> Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree
>>>> based pinctrl driver")
>>>>
>>>> Signed-off-by: Guo Mengqi <guomengqi3 at huawei.com>
>>> Good catch!
>>>
>>> I expect Tony to review the patch in-depth.
>> Thank you :)
> Thanks for looking into it. I wonder if we can rely on naming for
> pinmux_func_name_to_selector() though. Can things change in a way where
> we need to release everything and reparse? Mostly wondering what happens
> with DT overlays?

Hi

Let me confirm, you mean when the pin controller dtsi changed at 
runtime, some functions and groups can change silently while the dt-node 
name remains same, so the old data needs to be released and reparsed, right?

I don't know much about DT overlays. I can look deeper into revelant 
codes, maybe do some experiments too.

My guess now is DT overlay will first remove the old parsed nodes, then 
create new ones. If so, the modification to pcs_dt_node_to_map() in this 
patch is not affected.


Regards,

Mengqi

>>>> -static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
>>>> +int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
>>>>                                           const char *function)
>>>>    {
>>>>           const struct pinmux_ops *ops = pctldev->desc->pmxops;
>>> It appears you need to add EXPORT_SYMBOL_GPL() for this function
>>> so the module can build. (This is why the build robot complains.)
>> Yes, it happens when config=M. I will send a v2 patch later to fix this.
> That change might be worth doing in any case if there is need for it.
>
> Regards,
>
> Tony
>
> .