Re: [PATCH 3/7] x86: fix alloc_mptable

[Ingo Molnar <mingo@elte.hu>]

* Yinghai Lu <yinghai@kernel.org> wrote:

> fix the condition checking.
> 
> [ Impact: make alloc_mptable working ]
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/kernel/mpparse.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6/arch/x86/kernel/mpparse.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/mpparse.c
> +++ linux-2.6/arch/x86/kernel/mpparse.c
> @@ -873,21 +873,24 @@ inline void __init check_irq_src(struct
>  static int check_slot(unsigned long mpc_new_phys, unsigned long mpc_new_length,
>  		      int count)
>  {
> +	int ret = 0;
> +
>  	if (!mpc_new_phys) {
> -		pr_info("No spare slots, try to append...take your risk, "
> +		pr_warning("No spare slots, try to append...take your risk, "
>  			"new mpc_length %x\n", count);
>  	} else {
> -		if (count <= mpc_new_length)
> +		if (count <= mpc_new_length) {
>  			pr_info("No spare slots, try to append..., "
>  				"new mpc_length %x\n", count);
> -		else {
> +			ret = 1;
> +		} else {
>  			pr_err("mpc_new_length %lx is too small\n",
>  				mpc_new_length);
> -			return -1;
> +			ret = -1;
>  		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int  __init replace_intsrc_all(struct mpc_table *mpc,
> @@ -946,7 +949,7 @@ static int  __init replace_intsrc_all(st
>  		} else {
>  			struct mpc_intsrc *m = (struct mpc_intsrc *)mpt;
>  			count += sizeof(struct mpc_intsrc);
> -			if (!check_slot(mpc_new_phys, mpc_new_length, count))
> +			if (check_slot(mpc_new_phys, mpc_new_length, count) < 0)
>  				goto out;
>  			assign_to_mpc_intsrc(&mp_irqs[i], m);
>  			mpc->length = count;

hm, i modified this to the code attached below instead. Things like:

> -		pr_info("No spare slots, try to append...take your risk, "
> +		pr_warning("No spare slots, try to append...take your risk, "
>  			"new mpc_length %x\n", count);

are not acceptable at all. If there's _anything_ wrong with code 
like this, if we run out of a static pool of slots, we dont try to 
hack our way out of it... Instead we inform the user and bail out 
ASAP!

A predictable, well working way out of a resource shortage is _way_ 
more important than trying to clinge to some functionality and 
hoping that it will be all fine ...

So ... please describe under which conditions we can run out of 
slots, and what the options are in that situation.

	Ingo

------------------------------>
Subject: x86: fix alloc_mptable()
From: Yinghai Lu <yinghai@kernel.org>
Date: Wed, 06 May 2009 10:07:07 -0700

Fix the conditions when we stop updating the mptable due to
running out of slots.

[ Impact: fix memory corruption / non-working update_mptable boot parameter ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Len Brown <lenb@kernel.org>
LKML-Reference: <4A01C3BB.1000609@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/mpparse.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

Index: tip/arch/x86/kernel/mpparse.c
===================================================================
--- tip.orig/arch/x86/kernel/mpparse.c
+++ tip/arch/x86/kernel/mpparse.c
@@ -870,24 +870,17 @@ static
 inline void __init check_irq_src(struct mpc_intsrc *m, int *nr_m_spare) {}
 #endif /* CONFIG_X86_IO_APIC */
 
-static int check_slot(unsigned long mpc_new_phys, unsigned long mpc_new_length,
-		      int count)
+static int
+check_slot(unsigned long mpc_new_phys, unsigned long mpc_new_length, int count)
 {
-	if (!mpc_new_phys) {
-		pr_info("No spare slots, try to append...take your risk, "
-			"new mpc_length %x\n", count);
-	} else {
-		if (count <= mpc_new_length)
-			pr_info("No spare slots, try to append..., "
-				"new mpc_length %x\n", count);
-		else {
-			pr_err("mpc_new_length %lx is too small\n",
-				mpc_new_length);
-			return -1;
-		}
+	int ret = 0;
+
+	if (!mpc_new_phys || count <= mpc_new_length) {
+		WARN(1, "update_mptable: No spare slots (length: %x)\n", count);
+		return -1;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int  __init replace_intsrc_all(struct mpc_table *mpc,
@@ -946,7 +939,7 @@ static int  __init replace_intsrc_all(st
 		} else {
 			struct mpc_intsrc *m = (struct mpc_intsrc *)mpt;
 			count += sizeof(struct mpc_intsrc);
-			if (!check_slot(mpc_new_phys, mpc_new_length, count))
+			if (check_slot(mpc_new_phys, mpc_new_length, count) < 0)
 				goto out;
 			assign_to_mpc_intsrc(&mp_irqs[i], m);
 			mpc->length = count;