[PATCH] powerpc/pseries: Verify CPU doesn't exist before adding

[PATCH] powerpc/pseries: Verify CPU doesn't exist before adding

[Nathan Fontenot]

When DLPAR adding a CPU we should verify that the CPU does not already
exist. Failure to do so can generate a kernel oops;

[    9.465585] kernel BUG at arch/powerpc/platforms/pseries/dlpar.c:382!
[    9.465796] Oops: Exception in kernel mode, sig: 5 [#1]

This oops can be generated by causing a probe to be performed on a cpu
by writing to the sysfs cpu probe file (/sys/devices/system/cpu/probe).
This patch adds a check for the existence of cpu prior to probing the cpu
so userspace doing the wrong thing won't trigger a BUG_ON().

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c |   43 +++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index f244dcb..fe6320d 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -381,6 +381,32 @@ out:
 
 }
 
+static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
+{
+	struct device_node *child = NULL;
+	u32 my_drc_index;
+	bool found;
+	int rc;
+
+	/* Assume cpu doesn't exist */
+	found = false;
+
+	for_each_child_of_node(parent, child) {
+		rc = of_property_read_u32(child, "ibm,my-drc-index",
+					  &my_drc_index);
+		if (rc)
+			continue;
+
+		if (my_drc_index == drc_index) {
+			of_node_put(child);
+			found = true;
+			break;
+		}
+	}
+
+	return found;
+}
+
 static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
 {
 	struct device_node *dn, *parent;
@@ -391,14 +417,23 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
 	if (rc)
 		return -EINVAL;
 
-	rc = dlpar_acquire_drc(drc_index);
-	if (rc)
-		return -EINVAL;
-
 	parent = of_find_node_by_path("/cpus");
 	if (!parent)
 		return -ENODEV;
 
+	if (dlpar_cpu_exists(parent, drc_index)) {
+		of_node_put(parent);
+		printk(KERN_WARNING "CPU with drc index %x already exists\n",
+		       drc_index);
+		return -EINVAL;
+	}
+
+	rc = dlpar_acquire_drc(drc_index);
+	if (rc) {
+		of_node_put(parent);
+		return -EINVAL;
+	}
+
 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
 	of_node_put(parent);
 	if (!dn) {

Re: [PATCH] powerpc/pseries: Verify CPU doesn't exist before adding

[Denis Kirjanov]

On 10/23/15, Nathan Fontenot <nfont@linux.vnet.ibm.com> wrote:
> When DLPAR adding a CPU we should verify that the CPU does not already
> exist. Failure to do so can generate a kernel oops;
>
> [    9.465585] kernel BUG at arch/powerpc/platforms/pseries/dlpar.c:382!
> [    9.465796] Oops: Exception in kernel mode, sig: 5 [#1]
>
> This oops can be generated by causing a probe to be performed on a cpu
> by writing to the sysfs cpu probe file (/sys/devices/system/cpu/probe).
> This patch adds a check for the existence of cpu prior to probing the cpu
> so userspace doing the wrong thing won't trigger a BUG_ON().

Hi Nathan,

Can you please tell how to trigger the oops manually since I've tried to write
a core number to the probe file, but with no luck. Always get -EINVAL.

Thanks!
>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/dlpar.c |   43
> +++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c
> b/arch/powerpc/platforms/pseries/dlpar.c
> index f244dcb..fe6320d 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -381,6 +381,32 @@ out:
>
>  }
>
> +static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
> +{
> +	struct device_node *child = NULL;
> +	u32 my_drc_index;
> +	bool found;
> +	int rc;
> +
> +	/* Assume cpu doesn't exist */
> +	found = false;
> +
> +	for_each_child_of_node(parent, child) {
> +		rc = of_property_read_u32(child, "ibm,my-drc-index",
> +					  &my_drc_index);
> +		if (rc)
> +			continue;
> +
> +		if (my_drc_index == drc_index) {
> +			of_node_put(child);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	return found;
> +}
> +
>  static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
>  {
>  	struct device_node *dn, *parent;
> @@ -391,14 +417,23 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t
> count)
>  	if (rc)
>  		return -EINVAL;
>
> -	rc = dlpar_acquire_drc(drc_index);
> -	if (rc)
> -		return -EINVAL;
> -
>  	parent = of_find_node_by_path("/cpus");
>  	if (!parent)
>  		return -ENODEV;
>
> +	if (dlpar_cpu_exists(parent, drc_index)) {
> +		of_node_put(parent);
> +		printk(KERN_WARNING "CPU with drc index %x already exists\n",
> +		       drc_index);
> +		return -EINVAL;
> +	}
> +
> +	rc = dlpar_acquire_drc(drc_index);
> +	if (rc) {
> +		of_node_put(parent);
> +		return -EINVAL;
> +	}
> +
>  	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
>  	of_node_put(parent);
>  	if (!dn) {
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/pseries: Verify CPU doesn't exist before adding

[Nathan Fontenot]

On 10/25/2015 11:30 AM, Denis Kirjanov wrote:
> On 10/23/15, Nathan Fontenot <nfont@linux.vnet.ibm.com> wrote:
>> When DLPAR adding a CPU we should verify that the CPU does not already
>> exist. Failure to do so can generate a kernel oops;
>>
>> [    9.465585] kernel BUG at arch/powerpc/platforms/pseries/dlpar.c:382!
>> [    9.465796] Oops: Exception in kernel mode, sig: 5 [#1]
>>
>> This oops can be generated by causing a probe to be performed on a cpu
>> by writing to the sysfs cpu probe file (/sys/devices/system/cpu/probe).
>> This patch adds a check for the existence of cpu prior to probing the cpu
>> so userspace doing the wrong thing won't trigger a BUG_ON().
> 
> Hi Nathan,
> 
> Can you please tell how to trigger the oops manually since I've tried to write
> a core number to the probe file, but with no luck. Always get -EINVAL.

Triggering the oops manually may be a bit trickier than just writing to the sysfs file.

First, make sure you are writing the drc index of a cpu that is already present.
You can see a list of present cpus with 'lsslot -c cpu'.

You may be able to trigger the oops by just writing the drc index to the sysfs file
but will likely get -EINVAL since the probe code tries to acquire the cpu from
firmware and call configure-connector, both of which will likely fail before we
try to online the cpu and see the BUG_ON.

For a kvm guest, you should be able to generate the oops by killing the rtas_errd
daemon, do a cpu hotplug add from the host, then reboot the guest. If the
rtas_errd daemon is set to autostart you should see the oops at boot, otherwise
you can manually start rtas_errd and see the oops.

For a Power LPAR, the process would be much trickier. You would have to do some
hacking to the drmgr command (which write the drc index to the probe file) to
trick it into trying to add the same cpu twice when invoking cpu hotplug from
the HMC.

Hope that helps.

-Nathan 
> 
> Thanks!
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/dlpar.c |   43
>> +++++++++++++++++++++++++++++---
>>  1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/dlpar.c
>> b/arch/powerpc/platforms/pseries/dlpar.c
>> index f244dcb..fe6320d 100644
>> --- a/arch/powerpc/platforms/pseries/dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/dlpar.c
>> @@ -381,6 +381,32 @@ out:
>>
>>  }
>>
>> +static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
>> +{
>> +	struct device_node *child = NULL;
>> +	u32 my_drc_index;
>> +	bool found;
>> +	int rc;
>> +
>> +	/* Assume cpu doesn't exist */
>> +	found = false;
>> +
>> +	for_each_child_of_node(parent, child) {
>> +		rc = of_property_read_u32(child, "ibm,my-drc-index",
>> +					  &my_drc_index);
>> +		if (rc)
>> +			continue;
>> +
>> +		if (my_drc_index == drc_index) {
>> +			of_node_put(child);
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return found;
>> +}
>> +
>>  static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
>>  {
>>  	struct device_node *dn, *parent;
>> @@ -391,14 +417,23 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t
>> count)
>>  	if (rc)
>>  		return -EINVAL;
>>
>> -	rc = dlpar_acquire_drc(drc_index);
>> -	if (rc)
>> -		return -EINVAL;
>> -
>>  	parent = of_find_node_by_path("/cpus");
>>  	if (!parent)
>>  		return -ENODEV;
>>
>> +	if (dlpar_cpu_exists(parent, drc_index)) {
>> +		of_node_put(parent);
>> +		printk(KERN_WARNING "CPU with drc index %x already exists\n",
>> +		       drc_index);
>> +		return -EINVAL;
>> +	}
>> +
>> +	rc = dlpar_acquire_drc(drc_index);
>> +	if (rc) {
>> +		of_node_put(parent);
>> +		return -EINVAL;
>> +	}
>> +
>>  	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
>>  	of_node_put(parent);
>>  	if (!dn) {
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Re: powerpc/pseries: Verify CPU doesn't exist before adding

[Michael Ellerman]

On Fri, 2015-23-10 at 17:45:57 UTC, Nathan Fontenot wrote:
> When DLPAR adding a CPU we should verify that the CPU does not already
> exist. Failure to do so can generate a kernel oops;
> 
> [    9.465585] kernel BUG at arch/powerpc/platforms/pseries/dlpar.c:382!
> [    9.465796] Oops: Exception in kernel mode, sig: 5 [#1]
> 
> This oops can be generated by causing a probe to be performed on a cpu
> by writing to the sysfs cpu probe file (/sys/devices/system/cpu/probe).
> This patch adds a check for the existence of cpu prior to probing the cpu
> so userspace doing the wrong thing won't trigger a BUG_ON().
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1f859adb9253c2010799625822

cheers