Bharata B Rao <[email protected]> writes:
> The following warning is seen when a CPU is hot unplugged on a PowerKVM
> guest:
>
> refcount_t: underflow; use-after-free.
...
>
> Fix this by ensuring that of_node_put() is called only from the
> error path in dlpar_cpu_remove_by_index(). In the normal path,
> of_node_put() happens as part of dlpar_detach_node().
Don't we have the same bug in mobility.c ?
static int delete_dt_node(__be32 phandle)
{
struct device_node *dn;
dn = of_find_node_by_phandle(be32_to_cpu(phandle));
if (!dn)
return -ENOENT;
dlpar_detach_node(dn);
of_node_put(dn);
return 0;
}
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 7bc0e91..c5ed510 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -619,7 +619,8 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
> }
>
> rc = dlpar_cpu_remove(dn, drc_index);
> - of_node_put(dn);
> + if (rc)
> + of_node_put(dn);
> return rc;
> }
>
> @@ -856,9 +857,12 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t
> count)
> }
>
> rc = dlpar_cpu_remove(dn, drc_index);
> - of_node_put(dn);
> -
> - return rc ? rc : count;
> + if (rc) {
> + of_node_put(dn);
> + return rc;
> + } else {
> + return count;
> + }
> }
Which makes me think that the bug is actually that dlpar_detach_node()
should not be dropping the last reference on behalf of its caller. It
means that the caller has a pointer to a freed dn, which is wrong.
eg. A caller could do:
dlpar_detach_node(dn);
printk("detached %s\n", dn->name);
But dn will have been freed by the time the printk happens.
The problem is we call dlpar_detach_node() recursively, and for the
child nodes it *does* need to drop the last reference, because that's
the whole point. So it needs some refactoring.
I also notice there's no error checking on the removal of the child
nodes, which is pretty dubious:
child = of_get_next_child(dn, NULL);
while (child) {
dlpar_detach_node(child);
child = of_get_next_child(dn, child);
}
cheers
