All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND] NUMA x86: add constraints check for nid parameters
@ 2011-12-01 11:45 Petr Holasek
  2011-12-01 21:34 ` Andrew Morton
  0 siblings, 1 reply; 17+ messages in thread
From: Petr Holasek @ 2011-12-01 11:45 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, linux-kernel, Andrew Morton,
	Anton Arapov, Petr Holasek

This patch adds constraints checks into __node_distance() and
numa_set_distance() functions. If from or to parameters are
lower than zero, it results into oops now.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
---
 arch/x86/mm/numa.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index fbeaaf4..9e0ff2b 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -430,8 +430,9 @@ static int __init numa_alloc_distance(void)
  * calls are ignored until the distance table is reset with
  * numa_reset_distance().
  *
- * If @from or @to is higher than the highest known node at the time of
- * table creation or @distance doesn't make sense, the call is ignored.
+ * If @from or @to is higher than the highest known node or lower than zero
+ * at the time of table creation or @distance doesn't make sense, the call
+ * is ignored.
  * This is to allow simplification of specific NUMA config implementations.
  */
 void __init numa_set_distance(int from, int to, int distance)
@@ -439,7 +440,8 @@ void __init numa_set_distance(int from, int to, int distance)
 	if (!numa_distance && numa_alloc_distance() < 0)
 		return;
 
-	if (from >= numa_distance_cnt || to >= numa_distance_cnt) {
+	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
+			from < 0 || to < 0) {
 		printk_once(KERN_DEBUG "NUMA: Debug: distance out of bound, from=%d to=%d distance=%d\n",
 			    from, to, distance);
 		return;
@@ -457,7 +459,8 @@ void __init numa_set_distance(int from, int to, int distance)
 
 int __node_distance(int from, int to)
 {
-	if (from >= numa_distance_cnt || to >= numa_distance_cnt)
+	if (from < 0 || to < 0 ||
+			from >= numa_distance_cnt || to >= numa_distance_cnt)
 		return from == to ? LOCAL_DISTANCE : REMOTE_DISTANCE;
 	return numa_distance[from * numa_distance_cnt + to];
 }
-- 
1.7.6.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH RESEND] NUMA x86: add constraints check for nid parameters
  2011-12-01 11:45 [PATCH RESEND] NUMA x86: add constraints check for nid parameters Petr Holasek
@ 2011-12-01 21:34 ` Andrew Morton
  2011-12-01 23:14   ` Petr Holasek
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2011-12-01 21:34 UTC (permalink / raw)
  To: Petr Holasek
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, linux-kernel, Anton Arapov

On Thu,  1 Dec 2011 12:45:07 +0100
Petr Holasek <pholasek@redhat.com> wrote:

> This patch adds constraints checks into __node_distance() and
> numa_set_distance() functions. If from or to parameters are
> lower than zero, it results into oops now.

Passing negative numbers into __node_distance() sounds like a bug in
the caller, and this patch will remove our means of detecting that bug.

Perhaps we need to be told more about this patch.  Is the bug
user-triggerable?  If so, how?  How was this fault triggered? 
Etcetera.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: NUMA x86: add constraints check for nid parameters
  2011-12-01 21:34 ` Andrew Morton
@ 2011-12-01 23:14   ` Petr Holasek
  2011-12-01 23:28     ` Andrew Morton
  0 siblings, 1 reply; 17+ messages in thread
From: Petr Holasek @ 2011-12-01 23:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, linux-kernel, Anton Arapov

On Thu, 01 Dec 2011, Andrew Morton wrote:

> Date: Thu, 1 Dec 2011 13:34:51 -0800
> From: Andrew Morton <akpm@linux-foundation.org>
> To: Petr Holasek <pholasek@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
>  "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, Anton
>  Arapov <anton@redhat.com>
> Subject: Re: [PATCH RESEND] NUMA x86: add constraints check for nid
>  parameters
> 
> On Thu,  1 Dec 2011 12:45:07 +0100
> Petr Holasek <pholasek@redhat.com> wrote:
> 
> > This patch adds constraints checks into __node_distance() and
> > numa_set_distance() functions. If from or to parameters are
> > lower than zero, it results into oops now.
> 
> Passing negative numbers into __node_distance() sounds like a bug in
> the caller, and this patch will remove our means of detecting that bug.

That's true, but upper boundary is checked now, so why not to check lower?
Seems inconsistent to me - from this point of view even don't check anything
would be better for detecting bug in the caller.

> 
> Perhaps we need to be told more about this patch.  Is the bug
> user-triggerable?  If so, how?  How was this fault triggered? 
> Etcetera.
> 

AFAIK, neither __node_distance() nor numa_set_distance() aren't in any
path from user-space inputs. Their paramaters are based on ACPI tables
provided by HW vendors.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: NUMA x86: add constraints check for nid parameters
  2011-12-01 23:14   ` Petr Holasek
@ 2011-12-01 23:28     ` Andrew Morton
  2011-12-02 10:55       ` Petr Holasek
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2011-12-01 23:28 UTC (permalink / raw)
  To: Petr Holasek
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, linux-kernel, Anton Arapov

On Fri, 2 Dec 2011 00:14:42 +0100
Petr Holasek <pholasek@redhat.com> wrote:

> On Thu, 01 Dec 2011, Andrew Morton wrote:
> 
> > Date: Thu, 1 Dec 2011 13:34:51 -0800
> > From: Andrew Morton <akpm@linux-foundation.org>
> > To: Petr Holasek <pholasek@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
> >  "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, Anton
> >  Arapov <anton@redhat.com>
> > Subject: Re: [PATCH RESEND] NUMA x86: add constraints check for nid
> >  parameters
> > 
> > On Thu,  1 Dec 2011 12:45:07 +0100
> > Petr Holasek <pholasek@redhat.com> wrote:
> > 
> > > This patch adds constraints checks into __node_distance() and
> > > numa_set_distance() functions. If from or to parameters are
> > > lower than zero, it results into oops now.
> > 
> > Passing negative numbers into __node_distance() sounds like a bug in
> > the caller, and this patch will remove our means of detecting that bug.
> 
> That's true, but upper boundary is checked now, so why not to check lower?

Because it adds more code to the kernel and can hide bugs?

> Seems inconsistent to me - from this point of view even don't check anything
> would be better for detecting bug in the caller.
> 
> > 
> > Perhaps we need to be told more about this patch.  Is the bug
> > user-triggerable?  If so, how?  How was this fault triggered? 
> > Etcetera.
> > 
> 
> AFAIK, neither __node_distance() nor numa_set_distance() aren't in any
> path from user-space inputs. Their paramaters are based on ACPI tables
> provided by HW vendors.

That didn't answer my questions.  Have you observed any problems in
this code?  If so, please fully describe them.  Or was it purely from
code inspection?

If what we're doing here is to be defensive against buggy BIOS tables
(a good idea) then we should validate the BIOS table values as close as
possible to the point where they were read frmo the BIOS.  And we should
(probably) emit a warning if a bad table entry is detected, rather than
silently fixing it up.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: NUMA x86: add constraints check for nid parameters
  2011-12-01 23:28     ` Andrew Morton
@ 2011-12-02 10:55       ` Petr Holasek
  2011-12-06 20:45         ` David Rientjes
  0 siblings, 1 reply; 17+ messages in thread
From: Petr Holasek @ 2011-12-02 10:55 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, linux-kernel, Anton Arapov

On Thu, 01 Dec 2011, Andrew Morton wrote:
> 
> On Fri, 2 Dec 2011 00:14:42 +0100
> Petr Holasek <pholasek@redhat.com> wrote:
> 
> > On Thu, 01 Dec 2011, Andrew Morton wrote:
> > 
> > > On Thu,  1 Dec 2011 12:45:07 +0100
> > > Petr Holasek <pholasek@redhat.com> wrote:
> > > 
> > > > This patch adds constraints checks into __node_distance() and
> > > > numa_set_distance() functions. If from or to parameters are
> > > > lower than zero, it results into oops now.
> > > 
> > > Passing negative numbers into __node_distance() sounds like a bug in
> > > the caller, and this patch will remove our means of detecting that bug.
> > 
> > That's true, but upper boundary is checked now, so why not to check lower?
> 
> Because it adds more code to the kernel and can hide bugs?
> 
> > Seems inconsistent to me - from this point of view even don't check anything
> > would be better for detecting bug in the caller.
> > 
> > > 
> > > Perhaps we need to be told more about this patch.  Is the bug
> > > user-triggerable?  If so, how?  How was this fault triggered? 
> > > Etcetera.
> > > 
> > 
> > AFAIK, neither __node_distance() nor numa_set_distance() aren't in any
> > path from user-space inputs. Their paramaters are based on ACPI tables
> > provided by HW vendors.
> 
> That didn't answer my questions.  Have you observed any problems in
> this code?  If so, please fully describe them.  Or was it purely from
> code inspection?

Code inspection only, nothing from real life.

> 
> If what we're doing here is to be defensive against buggy BIOS tables
> (a good idea) then we should validate the BIOS table values as close as
> possible to the point where they were read frmo the BIOS.  And we should
> (probably) emit a warning if a bad table entry is detected, rather than
> silently fixing it up.

numa_set_distance() does exactly what you described above, only emits a
warning. I agree with your objections with __node_distance() checks, it
really can hide bugs in caller. So silent fix-up is the main problem and
we shouldn't check anything so the caller will be advised when using 
wrong nid by oops with a benefit of less code for us. Do I understand your
opinion on this type of code?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: NUMA x86: add constraints check for nid parameters
  2011-12-02 10:55       ` Petr Holasek
@ 2011-12-06 20:45         ` David Rientjes
  2011-12-07 19:40           ` Petr Holasek
  2011-12-07 19:43           ` [PATCH v2] " Petr Holasek
  0 siblings, 2 replies; 17+ messages in thread
From: David Rientjes @ 2011-12-06 20:45 UTC (permalink / raw)
  To: Petr Holasek
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

On Fri, 2 Dec 2011, Petr Holasek wrote:

> > > > > This patch adds constraints checks into __node_distance() and
> > > > > numa_set_distance() functions. If from or to parameters are
> > > > > lower than zero, it results into oops now.
> > > > 
> > > > Passing negative numbers into __node_distance() sounds like a bug in
> > > > the caller, and this patch will remove our means of detecting that bug.
> > > 
> > > That's true, but upper boundary is checked now, so why not to check lower?
> > 
> > Because it adds more code to the kernel and can hide bugs?
> > 

The upper bound is checked to ensure that we don't dereference past end of 
the array that stores the distance table, so it will catch errors for 
things like memory hotplug when additional nodes are onlined and the data 
structure isn't updated accordingly.

> > If what we're doing here is to be defensive against buggy BIOS tables
> > (a good idea) then we should validate the BIOS table values as close as
> > possible to the point where they were read frmo the BIOS.  And we should
> > (probably) emit a warning if a bad table entry is detected, rather than
> > silently fixing it up.
> 
> numa_set_distance() does exactly what you described above, only emits a
> warning. I agree with your objections with __node_distance() checks, it
> really can hide bugs in caller. So silent fix-up is the main problem and
> we shouldn't check anything so the caller will be advised when using 
> wrong nid by oops with a benefit of less code for us. Do I understand your
> opinion on this type of code?

I'd have no objection to adding a check to numa_set_distance() to ensure 
the node ids are non-negative in the same way we check that the distances 
themselves are non-negative; that can catch errors when pxms are used 
uninitialized when parsing the SRAT.  However, I think adding the check to 
__node_distance() is unnecessary.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: NUMA x86: add constraints check for nid parameters
  2011-12-06 20:45         ` David Rientjes
@ 2011-12-07 19:40           ` Petr Holasek
  2011-12-07 19:43           ` [PATCH v2] " Petr Holasek
  1 sibling, 0 replies; 17+ messages in thread
From: Petr Holasek @ 2011-12-07 19:40 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

On Tue, 06 Dec 2011, David Rientjes wrote:

> 
> On Fri, 2 Dec 2011, Petr Holasek wrote:
> 
> > > > > > This patch adds constraints checks into __node_distance() and
> > > > > > numa_set_distance() functions. If from or to parameters are
> > > > > > lower than zero, it results into oops now.
> > > > > 
> > > > > Passing negative numbers into __node_distance() sounds like a bug in
> > > > > the caller, and this patch will remove our means of detecting that bug.
> > > > 
> > > > That's true, but upper boundary is checked now, so why not to check lower?
> > > 
> > > Because it adds more code to the kernel and can hide bugs?
> > > 
> 
> The upper bound is checked to ensure that we don't dereference past end of 
> the array that stores the distance table, so it will catch errors for 
> things like memory hotplug when additional nodes are onlined and the data 
> structure isn't updated accordingly.

Thanks for clarification, I missed this case.

> 
> > > If what we're doing here is to be defensive against buggy BIOS tables
> > > (a good idea) then we should validate the BIOS table values as close as
> > > possible to the point where they were read frmo the BIOS.  And we should
> > > (probably) emit a warning if a bad table entry is detected, rather than
> > > silently fixing it up.
> > 
> > numa_set_distance() does exactly what you described above, only emits a
> > warning. I agree with your objections with __node_distance() checks, it
> > really can hide bugs in caller. So silent fix-up is the main problem and
> > we shouldn't check anything so the caller will be advised when using 
> > wrong nid by oops with a benefit of less code for us. Do I understand your
> > opinion on this type of code?
> 
> I'd have no objection to adding a check to numa_set_distance() to ensure 
> the node ids are non-negative in the same way we check that the distances 
> themselves are non-negative; that can catch errors when pxms are used 
> uninitialized when parsing the SRAT.  However, I think adding the check to 
> __node_distance() is unnecessary.

Ok, I'll try v2 without __node_distance()

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] NUMA x86: add constraints check for nid parameters
  2011-12-06 20:45         ` David Rientjes
  2011-12-07 19:40           ` Petr Holasek
@ 2011-12-07 19:43           ` Petr Holasek
  2011-12-07 21:46             ` David Rientjes
  2011-12-08 18:58             ` [PATCH v2] NUMA x86: add " Andi Kleen
  1 sibling, 2 replies; 17+ messages in thread
From: Petr Holasek @ 2011-12-07 19:43 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

This patch adds constraints checks for numa_set_distance()
function. It emits warning when pxms are used uninitialized
when parsing the SRAT.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
---
 arch/x86/mm/numa.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index fbeaaf4..42c12b6 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -430,8 +430,9 @@ static int __init numa_alloc_distance(void)
  * calls are ignored until the distance table is reset with
  * numa_reset_distance().
  *
- * If @from or @to is higher than the highest known node at the time of
- * table creation or @distance doesn't make sense, the call is ignored.
+ * If @from or @to is higher than the highest known node or lower than zero
+ * at the time of table creation or @distance doesn't make sense, the call
+ * is ignored.
  * This is to allow simplification of specific NUMA config implementations.
  */
 void __init numa_set_distance(int from, int to, int distance)
@@ -439,7 +440,8 @@ void __init numa_set_distance(int from, int to, int distance)
 	if (!numa_distance && numa_alloc_distance() < 0)
 		return;
 
-	if (from >= numa_distance_cnt || to >= numa_distance_cnt) {
+	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
+			from < 0 || to < 0) {
 		printk_once(KERN_DEBUG "NUMA: Debug: distance out of bound, from=%d to=%d distance=%d\n",
 			    from, to, distance);
 		return;
-- 
1.7.6.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] NUMA x86: add constraints check for nid parameters
  2011-12-07 19:43           ` [PATCH v2] " Petr Holasek
@ 2011-12-07 21:46             ` David Rientjes
  2011-12-08 12:16               ` [PATCH v3] " Petr Holasek
  2011-12-08 18:58             ` [PATCH v2] NUMA x86: add " Andi Kleen
  1 sibling, 1 reply; 17+ messages in thread
From: David Rientjes @ 2011-12-07 21:46 UTC (permalink / raw)
  To: Petr Holasek
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

On Wed, 7 Dec 2011, Petr Holasek wrote:

> This patch adds constraints checks for numa_set_distance()
> function. It emits warning when pxms are used uninitialized
> when parsing the SRAT.
> 

The changelog also needs to indicate that, in addition to emitting the 
warning, that this will avoid a store to a negative index in the 
numa_distance[] array, that's the real thing you're trying to catch.  This 
would happen if pxm_to_node() is returning NUMA_NO_NODE which is what the 
pxm-to-node mapping is initialized to.

> Signed-off-by: Petr Holasek <pholasek@redhat.com>
> ---
>  arch/x86/mm/numa.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index fbeaaf4..42c12b6 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -430,8 +430,9 @@ static int __init numa_alloc_distance(void)
>   * calls are ignored until the distance table is reset with
>   * numa_reset_distance().
>   *
> - * If @from or @to is higher than the highest known node at the time of
> - * table creation or @distance doesn't make sense, the call is ignored.
> + * If @from or @to is higher than the highest known node or lower than zero
> + * at the time of table creation or @distance doesn't make sense, the call
> + * is ignored.
>   * This is to allow simplification of specific NUMA config implementations.
>   */
>  void __init numa_set_distance(int from, int to, int distance)
> @@ -439,7 +440,8 @@ void __init numa_set_distance(int from, int to, int distance)
>  	if (!numa_distance && numa_alloc_distance() < 0)
>  		return;
>  
> -	if (from >= numa_distance_cnt || to >= numa_distance_cnt) {
> +	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
> +			from < 0 || to < 0) {
>  		printk_once(KERN_DEBUG "NUMA: Debug: distance out of bound, from=%d to=%d distance=%d\n",
>  			    from, to, distance);

That warning doesn't look appropriate for negative node ids, it should be 
modified to say the node ids or distance are out of bounds.  One more 
revision?

>  		return;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] NUMA x86: add constraints check for nid parameters
  2011-12-07 21:46             ` David Rientjes
@ 2011-12-08 12:16               ` Petr Holasek
  2011-12-08 21:09                 ` David Rientjes
  2011-12-09 20:08                 ` [tip:x86/mm] x86/numa: Add " tip-bot for Petr Holasek
  0 siblings, 2 replies; 17+ messages in thread
From: Petr Holasek @ 2011-12-08 12:16 UTC (permalink / raw)
  To: David Rientjes
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

This patch adds constraints checks for numa_set_distance()
function. It emits warning and avoids a store to a negative
index in numa_distance[] array. Negative ids can be passed when
pxm-to-nids mapping is not properly filled while parsing the SRAT.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
---
 arch/x86/mm/numa.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index fbeaaf4..cdc0054 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -430,8 +430,9 @@ static int __init numa_alloc_distance(void)
  * calls are ignored until the distance table is reset with
  * numa_reset_distance().
  *
- * If @from or @to is higher than the highest known node at the time of
- * table creation or @distance doesn't make sense, the call is ignored.
+ * If @from or @to is higher than the highest known node or lower than zero
+ * at the time of table creation or @distance doesn't make sense, the call
+ * is ignored.
  * This is to allow simplification of specific NUMA config implementations.
  */
 void __init numa_set_distance(int from, int to, int distance)
@@ -439,8 +440,9 @@ void __init numa_set_distance(int from, int to, int distance)
 	if (!numa_distance && numa_alloc_distance() < 0)
 		return;
 
-	if (from >= numa_distance_cnt || to >= numa_distance_cnt) {
-		printk_once(KERN_DEBUG "NUMA: Debug: distance out of bound, from=%d to=%d distance=%d\n",
+	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
+			from < 0 || to < 0) {
+		pr_warn_once("NUMA: Warning: node ids are out of bound, from=%d to=%d distance=%d\n",
 			    from, to, distance);
 		return;
 	}
-- 
1.7.6.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] NUMA x86: add constraints check for nid parameters
  2011-12-07 19:43           ` [PATCH v2] " Petr Holasek
  2011-12-07 21:46             ` David Rientjes
@ 2011-12-08 18:58             ` Andi Kleen
  2011-12-08 21:12               ` David Rientjes
  2011-12-09  7:11               ` Ingo Molnar
  1 sibling, 2 replies; 17+ messages in thread
From: Andi Kleen @ 2011-12-08 18:58 UTC (permalink / raw)
  To: Petr Holasek
  Cc: David Rientjes, Andrew Morton, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, linux-kernel, Anton Arapov

Petr Holasek <pholasek@redhat.com> writes:

> This patch adds constraints checks for numa_set_distance()
> function. It emits warning when pxms are used uninitialized
> when parsing the SRAT.

Can you expand on the motivation for this please?
Is this to catch buggy code or to catch buggy SRATs?
If the later are they actually out there?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] NUMA x86: add constraints check for nid parameters
  2011-12-08 12:16               ` [PATCH v3] " Petr Holasek
@ 2011-12-08 21:09                 ` David Rientjes
  2011-12-09  7:10                   ` Ingo Molnar
  2011-12-09 20:08                 ` [tip:x86/mm] x86/numa: Add " tip-bot for Petr Holasek
  1 sibling, 1 reply; 17+ messages in thread
From: David Rientjes @ 2011-12-08 21:09 UTC (permalink / raw)
  To: Petr Holasek
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	linux-kernel, Anton Arapov

On Thu, 8 Dec 2011, Petr Holasek wrote:

> This patch adds constraints checks for numa_set_distance()
> function. It emits warning and avoids a store to a negative
> index in numa_distance[] array. Negative ids can be passed when
> pxm-to-nids mapping is not properly filled while parsing the SRAT.
> 
> Signed-off-by: Petr Holasek <pholasek@redhat.com>

Acked-by: David Rientjes <rientjes@google.com>

Thanks Petr!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] NUMA x86: add constraints check for nid parameters
  2011-12-08 18:58             ` [PATCH v2] NUMA x86: add " Andi Kleen
@ 2011-12-08 21:12               ` David Rientjes
  2011-12-09  7:11               ` Ingo Molnar
  1 sibling, 0 replies; 17+ messages in thread
From: David Rientjes @ 2011-12-08 21:12 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Petr Holasek, Andrew Morton, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, linux-kernel, Anton Arapov

On Thu, 8 Dec 2011, Andi Kleen wrote:

> > This patch adds constraints checks for numa_set_distance()
> > function. It emits warning when pxms are used uninitialized
> > when parsing the SRAT.
> 
> Can you expand on the motivation for this please?

It's adding a debugging check if something erroneously passes a negative 
node id when setting a distance, perhaps because of buggy code that failed 
to initialize the pxm-to-node mapping correctly which would result in 
storing to a negative index of the numa_distance array as indicated in the 
changelog.  It's for robustness and to warn explicitly rather than 
silently rejecting it unless you're monitoring debug output.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] NUMA x86: add constraints check for nid parameters
  2011-12-08 21:09                 ` David Rientjes
@ 2011-12-09  7:10                   ` Ingo Molnar
  2011-12-09 10:44                     ` David Rientjes
  0 siblings, 1 reply; 17+ messages in thread
From: Ingo Molnar @ 2011-12-09  7:10 UTC (permalink / raw)
  To: David Rientjes
  Cc: Petr Holasek, Andrew Morton, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, linux-kernel, Anton Arapov


* David Rientjes <rientjes@google.com> wrote:

> On Thu, 8 Dec 2011, Petr Holasek wrote:
> 
> > This patch adds constraints checks for numa_set_distance()
> > function. It emits warning and avoids a store to a negative
> > index in numa_distance[] array. Negative ids can be passed when
> > pxm-to-nids mapping is not properly filled while parsing the SRAT.
> > 
> > Signed-off-by: Petr Holasek <pholasek@redhat.com>
> 
> Acked-by: David Rientjes <rientjes@google.com>

Thanks guys.

Just to double check: there's no known instance of such a bad 
SRAT in existence, so this commit can wait until v3.3 and does 
hot have to go into v3.2, right?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] NUMA x86: add constraints check for nid parameters
  2011-12-08 18:58             ` [PATCH v2] NUMA x86: add " Andi Kleen
  2011-12-08 21:12               ` David Rientjes
@ 2011-12-09  7:11               ` Ingo Molnar
  1 sibling, 0 replies; 17+ messages in thread
From: Ingo Molnar @ 2011-12-09  7:11 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Petr Holasek, David Rientjes, Andrew Morton, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, linux-kernel, Anton Arapov


* Andi Kleen <andi@firstfloor.org> wrote:

> Petr Holasek <pholasek@redhat.com> writes:
> 
> > This patch adds constraints checks for numa_set_distance()
> > function. It emits warning when pxms are used uninitialized
> > when parsing the SRAT.
> 
> Can you expand on the motivation for this please?
> Is this to catch buggy code or to catch buggy SRATs?

Both.

> If the later are they actually out there?

Not that i know of - they would likely not boot Linux very well 
today.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] NUMA x86: add constraints check for nid parameters
  2011-12-09  7:10                   ` Ingo Molnar
@ 2011-12-09 10:44                     ` David Rientjes
  0 siblings, 0 replies; 17+ messages in thread
From: David Rientjes @ 2011-12-09 10:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Petr Holasek, Andrew Morton, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, linux-kernel, Anton Arapov

On Fri, 9 Dec 2011, Ingo Molnar wrote:

> Thanks guys.
> 
> Just to double check: there's no known instance of such a bad 
> SRAT in existence, so this commit can wait until v3.3 and does 
> hot have to go into v3.2, right?
> 

Andi mentioned buggy SRATs, but it's outside the scope of this patch; this 
has nothing to do with buggy SRATs.

Ignoring NUMA emulation which plays with these when you boot with 
numa=fake, this patch is catching any instance where pxm_to_node(x) 
returns NUMA_NO_NODE, meaning x is not an initialized pxm.  For x86, we 
initialize pxms by grabbing them from the SRAT, picking the first unused 
node id, and mapping them.  The SRAT has nothing to do with this, the ACPI 
spec has no notion of what we've defined a node to be.  So Petr's patch 
will simply catch any instance of pxm_to_node(x) where x was not 
initialized and incorrectly referencing 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [tip:x86/mm] x86/numa: Add constraints check for nid parameters
  2011-12-08 12:16               ` [PATCH v3] " Petr Holasek
  2011-12-08 21:09                 ` David Rientjes
@ 2011-12-09 20:08                 ` tip-bot for Petr Holasek
  1 sibling, 0 replies; 17+ messages in thread
From: tip-bot for Petr Holasek @ 2011-12-09 20:08 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, anton, pholasek, hpa, mingo, tglx, rientjes, mingo

Commit-ID:  54eed6cb16ec315565aaaf8e34252ca253a68b7b
Gitweb:     http://git.kernel.org/tip/54eed6cb16ec315565aaaf8e34252ca253a68b7b
Author:     Petr Holasek <pholasek@redhat.com>
AuthorDate: Thu, 8 Dec 2011 13:16:41 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 9 Dec 2011 08:03:34 +0100

x86/numa: Add constraints check for nid parameters

This patch adds constraint checks to the numa_set_distance()
function.

When the check triggers (this should not happen normally) it
emits a warning and avoids a store to a negative index in
numa_distance[] array - i.e. avoids memory corruption.

Negative ids can be passed when the pxm-to-nids mapping is not
properly filled while parsing the SRAT.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Anton Arapov <anton@redhat.com>
Link: http://lkml.kernel.org/r/20111208121640.GA2229@dhcp-27-244.brq.redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/mm/numa.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index fbeaaf4..cdc0054 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -430,8 +430,9 @@ static int __init numa_alloc_distance(void)
  * calls are ignored until the distance table is reset with
  * numa_reset_distance().
  *
- * If @from or @to is higher than the highest known node at the time of
- * table creation or @distance doesn't make sense, the call is ignored.
+ * If @from or @to is higher than the highest known node or lower than zero
+ * at the time of table creation or @distance doesn't make sense, the call
+ * is ignored.
  * This is to allow simplification of specific NUMA config implementations.
  */
 void __init numa_set_distance(int from, int to, int distance)
@@ -439,8 +440,9 @@ void __init numa_set_distance(int from, int to, int distance)
 	if (!numa_distance && numa_alloc_distance() < 0)
 		return;
 
-	if (from >= numa_distance_cnt || to >= numa_distance_cnt) {
-		printk_once(KERN_DEBUG "NUMA: Debug: distance out of bound, from=%d to=%d distance=%d\n",
+	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
+			from < 0 || to < 0) {
+		pr_warn_once("NUMA: Warning: node ids are out of bound, from=%d to=%d distance=%d\n",
 			    from, to, distance);
 		return;
 	}

^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2011-12-09 20:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-01 11:45 [PATCH RESEND] NUMA x86: add constraints check for nid parameters Petr Holasek
2011-12-01 21:34 ` Andrew Morton
2011-12-01 23:14   ` Petr Holasek
2011-12-01 23:28     ` Andrew Morton
2011-12-02 10:55       ` Petr Holasek
2011-12-06 20:45         ` David Rientjes
2011-12-07 19:40           ` Petr Holasek
2011-12-07 19:43           ` [PATCH v2] " Petr Holasek
2011-12-07 21:46             ` David Rientjes
2011-12-08 12:16               ` [PATCH v3] " Petr Holasek
2011-12-08 21:09                 ` David Rientjes
2011-12-09  7:10                   ` Ingo Molnar
2011-12-09 10:44                     ` David Rientjes
2011-12-09 20:08                 ` [tip:x86/mm] x86/numa: Add " tip-bot for Petr Holasek
2011-12-08 18:58             ` [PATCH v2] NUMA x86: add " Andi Kleen
2011-12-08 21:12               ` David Rientjes
2011-12-09  7:11               ` Ingo Molnar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.