[FIX PATCH v1] powerpc/pseries: Fix reference count leak during CPU unplug

[Michael Ellerman]

Bharata B Rao <bharata at linux.vnet.ibm.com> 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