Re: [PATCH] powerpc: Fix device node refcounting

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

On 2/7/23 9:14 AM, Nathan Lynch wrote:
> 
> (cc'ing a few possibly interested people)
> 
> 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. 

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

Thanks,

Brian

> 
> Just hoping we're not stepping into a situation where we're preventing
> leaks in some situations but doing use-after-free in others. :-)
> 
>>
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/reconfig.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
>> index 599bd2c78514..8cb7309b19a4 100644
>> --- a/arch/powerpc/platforms/pseries/reconfig.c
>> +++ b/arch/powerpc/platforms/pseries/reconfig.c
>> @@ -77,6 +77,7 @@ static int pSeries_reconfig_remove_node(struct device_node *np)
>>  	}
>>  
>>  	of_detach_node(np);
>> +	of_node_put(np);
>>  	of_node_put(parent);
>>  	return 0;
> 
> In a situation like this where the of_node_put() call isn't obviously
> connected to one of the of_ iterator APIs or similar, I would prefer a
> comment indicating which "get" it balances. I suppose it corresponds to
> the node initialization itself, i.e. the of_node_init() call sites in
> pSeries_reconfig_add_node() and drivers/of/fdt.c::populate_node().

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