答复: Re: [PATCH v2 01/15] ARM: actions: fix a leaked reference byaddingmissing of_node_put

答复: Re: [PATCH v2 01/15] ARM: actions: fix a leaked reference byaddingmissing of_node_put

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 5085 bytes --]

>>>> The call to of_get_next_child returns a node pointer with refcount
>>>> incremented thus it must be explicitly decremented after the last
>>>> usage.
>>>>
>>>> Detected by coccinelle with the following warnings:
>>>> ./arch/arm/mach-actions/platsmp.c:112:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 103, but without a corresponding object release within this function.
>>>> ./arch/arm/mach-actions/platsmp.c:124:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 115, but without a corresponding object release within this function.
>>>> ./arch/arm/mach-actions/platsmp.c:137:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 128, but without a corresponding object release within this function.
>>>
>>> I question this.  Your reasoning is that the node is no longer used
>>> so the reference count needs to be put.
>>>
>>> However, in all these cases, data is read from the nodes properties
>>> and the device remains in-use for the life of the kernel.  There is
>>> a big difference here.
>>>
>>> With normal drivers, each device is bound to their associated device
>>> node associated with the device.  When the device node goes away, then
>>> the corresponding device goes away too, which causes the driver to be
>>> unbound from the device.
>>>
>>> However, there is another class of "driver" which are the ones below,
>>> where they are "permanent" devices.  These can never go away, even if
>>> the device node refcount hits zero and the device node is freed - the
>>> device is still present and in-use in the system.
>>
>> Hi Russel,
>> Thank you very much for your comments.
>> The problem we want to solve is "fix the reference leaks of device_node".
>> We use the following coccinelle script to achieve the goal:
>> @search exists@
>> local idexpression struct device_node * node;
>> expression e, e1;
>> position p1,p2;
>> type T,T1,T2;
>> @@
>>
>> node = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
>>     of_find_node_by_type\|of_find_node_by_name\|of_find_all_nodes\|
>>     of_get_cpu_node\|of_get_parent\|of_get_next_parent\|
>>     of_get_next_child\|of_get_next_available_child\|of_get_next_cpu_node\|
>>     of_get_compatible_child\|of_get_child_by_name\|of_find_node_opts_by_path\|
>>     of_find_node_with_property\|of_find_matching_node_and_match\|of_find_node_by_phandle\|
>>     of_parse_phandle\)(...)
>> ... when != e = (T)node
>> if (node == NULL || ...) { ... return ...; }
>> ... when != of_node_put(node)
>>     when != if (node) { ... of_node_put(node) ... }
>>     when != e1 = (T1)node
>> (
>>   return (T2)node;
>> | return@p2 ...;
>> )
>>
>> Using the script above, we found the issues for this file in the patch:
>> arch/arm/mach-actions/platsmp.c
>> 99 static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
>> 100 {
>> 101 struct device_node *node;
>> 102
>> 103 node = of_find_compatible_node(NULL, NULL, "actions,s500-timer");
>> ...
>> 109 timer_base_addr = of_iomap(node, 0);
>> 110 if (!timer_base_addr) {
>> 111 pr_err("%s: could not map timer registers\n", __func__);
>> 112 return;
>> 113 }
>> 114
>> 115 node = of_find_compatible_node(NULL, NULL, "actions,s500-sps");
>> ...
>>
>> The comment for of_find_compatible_node writes:
>> “Returns a node pointer with refcount incremented, use of_node_put() on it when done.”
>> the node is a local variable obtained by of_find_compatible_node.
>> But the code does not call on_node_put() to reduce the reference count,
>> the function returns directly, or directly reassigns it.
>> This leads to a reference leak.
>
> The unanswered question I raised a couple months ago when this topic
> first came up was: does it actually matter? The function seemed
> non-reentrant, and the base Device Tree stays around throughout the
> lifetime of the kernel anyway. So it seems you're fixing warnings that
> in practice don't make a difference - it won't hurt to put the nodes,
> but I haven't seen a use case description of how the current code leads
> to a leak - when the system powers down, the device_node reference count
> shouldn't matter any more?
>
> If it does lead to an actual problem, then the patch fixing it should
> get a Fixes header so that it gets backported to all relevant upstream
> and downstream kernels.
>
Hi Andreas,
Thank you for your comments.
There may be three points.
1, no consideration of of_node_put()
Many of our series of patches are fixed it.

2, considering of_node_put(), but some branches are missing.
In our series of patches, there are also fixes for it.
https://lkml.org/lkml/2019/3/5/243
as well as
https://lkml.org/lkml/2019/3/5/224

When suspend/resume is supported, there may be an actual leak.

3, When adding drivers, we will generally refer to the existing
stable and tested code in the kernel, but if the existing code 
is inconsistent with the processing of of_node_put(), 
we may be confused.

If we fix them all, it might be good for new developers.

Thanks and regards,
 Wen

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel