Re: [PATCH] powerpc: Fix device node refcounting

[Brian King <brking@linux.vnet.ibm.com>]

On 2/9/23 11:11 AM, Nathan Lynch wrote:
> Brian King <brking@linux.vnet.ibm.com> writes:
>> On 2/7/23 9:14 AM, Nathan Lynch wrote:
>>> Brian King <brking@linux.vnet.ibm.com> writes:
>>>> While testing fixes to the hvcs hotplug code, kmemleak was reporting
>>>> potential memory leaks. This was tracked down to the struct device_node
>>>> object associated with the hvcs device. Looking at the leaked
>>>> object in crash showed that the kref in the kobject in the device_node
>>>> had a reference count of 1 still, and the release function was never
>>>> getting called as a result of this. This adds an of_node_put in
>>>> pSeries_reconfig_remove_node in order to balance the refcounting
>>>> so that we actually free the device_node in the case of it being
>>>> allocated in pSeries_reconfig_add_node.
>>>
>>> My concern here would be whether the additional put is the right thing
>>> to do in all cases. The questions it raises for me are:
>>>
>>> - Is it safe for nodes that were present at boot, instead of added
>>>   dynamically?
>>
>> Yes. of_node_release has a check to see if OF_DYNAMIC is set. If it is not set,
>> the release function is a noop.
> 
> Yes, but to be more specific - does the additional of_node_put() risk
> underflowing the refcount on nodes without the OF_DYNAMIC flag? I
> suspect it's OK. If it's not, then I would expect to see warnings from
> the refcount code when that case is exercised.

Agreed. I have not seen any refcount underflow warnings in the testing I've done
so far.

> 
>>
>>> - Is it correct for all types of nodes, or is there something specific
>>>   to hvcs that leaves a dangling refcount?
>>
>> I would welcome more testing and I shared the same concern. I did do some
>> DLPARs of a virtual ethernet device with the change along with CONFIG_PAGE_POISONING
>> enabled and did not run into any issues. However if I do a DLPAR remove of a virtual
>> ethernet device without the change with kmemleak enabled it does not detect any
>> leaked memory.
> 
> Seems odd. If the change is generically correct, then without it applied
> I would expect kmemleak to flag a leak on removal of any type of
> dynamically-added node. On the other hand, if the change is for some
> reason not correct for virtual ethernet devices, then I would expect it
> to cause complaints from the refcount code and/or allocator debug
> facilities. But if I understand correctly, neither of those things is
> happening.

Agreed. I'll do some more testing with and without the change and see
what that yields.

-Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center