Re: [PATCH] powerpc: Fix device node refcounting

[Nathan Lynch <nathanl@linux.ibm.com>]

(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?
- Is it correct for all types of nodes, or is there something specific
  to hvcs that leaves a dangling refcount?

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