[Regression] 2.6.33-rc2 - pci: Commit e0cd516 causes OOPS

[Regression] 2.6.33-rc2 - pci: Commit e0cd516 causes OOPS

[David John]

Hi All,

Commit e0cd516 causes an null pointer dereference when reading from the
sysfs attributes local_cpu* on Intel machines with no ACPI NUMA
proximity info, since dev->numa_node gets set to -1 for all PCI devices,
which then gets passed to cpumask_of_node.

The patch following this mail fixes the problem for x86. Perhaps a more
thorough solution would be to fix the PCI layer to set the node
information for devices to zero rather than -1 (Since if CONFIG_NUMA=y
we have node 0)? I don't know whether it is safe / correct to do this.

Regards,
David.

[PATCH] Check the node argument passed to cpumask_of_node

[David John]

Ensure that the node value is valid.

Signed-off-by: David John <davidjon@xenontk.org>

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index c5087d7..1141a6e 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -99,7 +99,8 @@ extern const struct cpumask *cpumask_of_node(int node);
 /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
 static inline const struct cpumask *cpumask_of_node(int node)
 {
-	return node_to_cpumask_map[node];
+	return (node < 0 || node >= nr_node_ids) ? cpu_online_mask :
+						   node_to_cpumask_map[node];
 }
 #endif
 

Re: [Regression] 2.6.33-rc2 - pci: Commit e0cd516 causes OOPS

[Rafael J. Wysocki]

On Friday 01 January 2010, David John wrote:
> Hi All,

Hi,

> Commit e0cd516 causes an null pointer dereference when reading from the
> sysfs attributes local_cpu* on Intel machines with no ACPI NUMA
> proximity info, since dev->numa_node gets set to -1 for all PCI devices,
> which then gets passed to cpumask_of_node.
> 
> The patch following this mail fixes the problem for x86. Perhaps a more
> thorough solution would be to fix the PCI layer to set the node
> information for devices to zero rather than -1 (Since if CONFIG_NUMA=y
> we have node 0)? I don't know whether it is safe / correct to do this.

First, it would be nicer if you said what the commit subject was in addition
to providing its hash.

Second, why don't you put the information above into the changelog of your
patch?

Rafael

Re: [Regression] 2.6.33-rc2 - pci: Commit e0cd516 causes OOPS

[Yinghai Lu]

On Fri, Jan 1, 2010 at 11:50 AM, David John <davidjon@xenontk.org> wrote:
> Hi All,
>
> Commit e0cd516 causes an null pointer dereference when reading from the
> sysfs attributes local_cpu* on Intel machines with no ACPI NUMA
> proximity info, since dev->numa_node gets set to -1 for all PCI devices,
> which then gets passed to cpumask_of_node.
>
> The patch following this mail fixes the problem for x86. Perhaps a more
> thorough solution would be to fix the PCI layer to set the node
> information for devices to zero rather than -1 (Since if CONFIG_NUMA=y
> we have node 0)? I don't know whether it is safe / correct to do this.

no.

1. -1, mean calling code will use node that code is running on.
2. the system that have two or more nodes, and more peer root buses.
if the first node doesn't have RAM installed, no node0 then.

YH

[PATCH v2] Check the node argument passed to cpumask_of_node

[David John]

Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus' NUMA information"
causes an null pointer dereference when reading from the sysfs attributes local_cpu*
on Intel machines with no ACPI NUMA proximity info, since dev->numa_node gets set to -1
for all PCI devices, which then gets passed to cpumask_of_node.

Ensure that the node value is valid.

Signed-off-by: David John <davidjon@xenontk.org>

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index c5087d7..1141a6e 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -99,7 +99,8 @@ extern const struct cpumask *cpumask_of_node(int node);
 /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
 static inline const struct cpumask *cpumask_of_node(int node)
 {
-	return node_to_cpumask_map[node];
+	return (node < 0 || node >= nr_node_ids) ? cpu_online_mask :
+						   node_to_cpumask_map[node];
 }
 #endif
 

Re: [Regression] 2.6.33-rc2 - pci: Commit e0cd516 causes OOPS

[David John]

On 01/02/2010 03:52 AM, Yinghai Lu wrote:
> On Fri, Jan 1, 2010 at 11:50 AM, David John <davidjon@xenontk.org> wrote:
>> Hi All,
>>
>> Commit e0cd516 causes an null pointer dereference when reading from the
>> sysfs attributes local_cpu* on Intel machines with no ACPI NUMA
>> proximity info, since dev->numa_node gets set to -1 for all PCI devices,
>> which then gets passed to cpumask_of_node.
>>
>> The patch following this mail fixes the problem for x86. Perhaps a more
>> thorough solution would be to fix the PCI layer to set the node
>> information for devices to zero rather than -1 (Since if CONFIG_NUMA=y
>> we have node 0)? I don't know whether it is safe / correct to do this.
> 
> no.
> 
> 1. -1, mean calling code will use node that code is running on.
> 2. the system that have two or more nodes, and more peer root buses.
> if the first node doesn't have RAM installed, no node0 then.
> 
> YH
> 

Oh I see. Thanks.

Regards,
David.

Re: [PATCH v2] Check the node argument passed to cpumask_of_node

[Rusty Russell]

On Sat, 2 Jan 2010 04:45:47 pm David John wrote:
> Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus' NUMA information"
> causes an null pointer dereference when reading from the sysfs attributes local_cpu*
> on Intel machines with no ACPI NUMA proximity info, since dev->numa_node gets set to -1
> for all PCI devices, which then gets passed to cpumask_of_node.
> 
> Ensure that the node value is valid.

This only works for x86, and only for !CONFIG_DEBUG_PER_CPU_MAPS.

I suggest fixing the callers introduced in e0cd516 for the moment,
then if you feel enthused, change the semantics of cpumask_of_node and
remove the checks from the various callers.

(And please only check for -1: it has a special meaning, unlike other
invalid numbers which indicate a bug).

Thanks,
Rusty.

Re: [PATCH v2] Check the node argument passed to cpumask_of_node

[Jin Dongming]

Hi, David

This problem also happened when the CONFIG_DEBUG_PER_CPU_MAPS was used,
so how about modifying the code for it working well?

Best Regards,
Jin Dongming

David John wrote:
> Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus' NUMA information"
> causes an null pointer dereference when reading from the sysfs attributes local_cpu*
> on Intel machines with no ACPI NUMA proximity info, since dev->numa_node gets set to -1
> for all PCI devices, which then gets passed to cpumask_of_node.
> 
> Ensure that the node value is valid.
> 
> Signed-off-by: David John <davidjon@xenontk.org>
> 
> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> index c5087d7..1141a6e 100644
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -99,7 +99,8 @@ extern const struct cpumask *cpumask_of_node(int node);
>  /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
>  static inline const struct cpumask *cpumask_of_node(int node)
>  {
> -	return node_to_cpumask_map[node];
> +	return (node < 0 || node >= nr_node_ids) ? cpu_online_mask :
> +						   node_to_cpumask_map[node];
>  }
>  #endif
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH v2] Check the node argument passed to cpumask_of_node

[David John]

On 01/04/2010 09:58 AM, Rusty Russell wrote:
> On Sat, 2 Jan 2010 04:45:47 pm David John wrote:
>> Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus' NUMA information"
>> causes an null pointer dereference when reading from the sysfs attributes local_cpu*
>> on Intel machines with no ACPI NUMA proximity info, since dev->numa_node gets set to -1
>> for all PCI devices, which then gets passed to cpumask_of_node.
>>
>> Ensure that the node value is valid.
> 
> This only works for x86, and only for !CONFIG_DEBUG_PER_CPU_MAPS.
> 
> I suggest fixing the callers introduced in e0cd516 for the moment,
> then if you feel enthused, change the semantics of cpumask_of_node and
> remove the checks from the various callers.

Okay, I'll do as you suggested.

Regards,
David.

[PATCH v3] Check the node argument passed to cpumask_of_node

[David John]

Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus' NUMA information"
causes an null pointer dereference when reading from the sysfs attributes local_cpu*
on Intel machines with no ACPI NUMA proximity info, since dev->numa_node gets set to -1
for all PCI devices, which then gets passed to cpumask_of_node.

Add a check to prevent this.

Signed-off-by: David John <davidjon@xenontk.org>

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c5df94e..807224e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -75,7 +75,8 @@ static ssize_t local_cpus_show(struct device *dev,
 	int len;
 
 #ifdef CONFIG_NUMA
-	mask = cpumask_of_node(dev_to_node(dev));
+	mask = (dev_to_node(dev) == -1) ? cpu_online_mask :
+					  cpumask_of_node(dev_to_node(dev));
 #else
 	mask = cpumask_of_pcibus(to_pci_dev(dev)->bus);
 #endif
@@ -93,7 +94,8 @@ static ssize_t local_cpulist_show(struct device *dev,
 	int len;
 
 #ifdef CONFIG_NUMA
-	mask = cpumask_of_node(dev_to_node(dev));
+	mask = (dev_to_node(dev) == -1) ? cpu_online_mask :
+					  cpumask_of_node(dev_to_node(dev));
 #else
 	mask = cpumask_of_pcibus(to_pci_dev(dev)->bus);
 #endif

Re: [PATCH v3] Check the node argument passed to cpumask_of_node

[Jesse Barnes]

On Mon,  4 Jan 2010 20:28:57 +0530
David John <davidjon@xenontk.org> wrote:

> Commit e0cd516 "PCI: derive nearby CPUs from device's instead of bus'
> NUMA information" causes an null pointer dereference when reading
> from the sysfs attributes local_cpu* on Intel machines with no ACPI
> NUMA proximity info, since dev->numa_node gets set to -1 for all PCI
> devices, which then gets passed to cpumask_of_node.
> 
> Add a check to prevent this.
> 
> Signed-off-by: David John <davidjon@xenontk.org>
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c5df94e..807224e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -75,7 +75,8 @@ static ssize_t local_cpus_show(struct device *dev,
>  	int len;
>  
>  #ifdef CONFIG_NUMA
> -	mask = cpumask_of_node(dev_to_node(dev));
> +	mask = (dev_to_node(dev) == -1) ? cpu_online_mask :
> +
> cpumask_of_node(dev_to_node(dev)); #else
>  	mask = cpumask_of_pcibus(to_pci_dev(dev)->bus);
>  #endif
> @@ -93,7 +94,8 @@ static ssize_t local_cpulist_show(struct device
> *dev, int len;
>  
>  #ifdef CONFIG_NUMA
> -	mask = cpumask_of_node(dev_to_node(dev));
> +	mask = (dev_to_node(dev) == -1) ? cpu_online_mask :
> +
> cpumask_of_node(dev_to_node(dev)); #else
>  	mask = cpumask_of_pcibus(to_pci_dev(dev)->bus);
>  #endif
> 

Applied to my for-linus branch, thanks David.

-- 
Jesse Barnes, Intel Open Source Technology Center