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