Re: [PATCH] x86: Fix potential overflow in perfctr reservation

Re: [PATCH] x86: Fix potential overflow in perfctr reservation

[Andrew Morton]

On Tue, 17 Apr 2007 23:59:10 GMT Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1714f9bfc92d6ee67e84127332a1fae27772acfe
> Commit:     1714f9bfc92d6ee67e84127332a1fae27772acfe
> Parent:     08269c6d38e003adb12f55c6d795daa89bdc1bae
> Author:     Andi Kleen <ak@suse.de>
> AuthorDate: Mon Apr 16 10:30:27 2007 +0200
> Committer:  Andi Kleen <andi@basil.nowhere.org>
> CommitDate: Mon Apr 16 10:30:27 2007 +0200
> 
>     [PATCH] x86: Fix potential overflow in perfctr reservation
>     
>     While reviewing this code again I found a potential overflow of the bitmap.
>     The p4 oprofile can theoretically set bits beyond the reservation bitmap for
>     specific configurations. Avoid that by sizing the bitmaps properly.
>     
>     Signed-off-by: Andi Kleen <ak@suse.de>
> ---
>  arch/i386/kernel/nmi.c   |    9 +++++----
>  arch/x86_64/kernel/nmi.c |   10 ++++++----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/i386/kernel/nmi.c b/arch/i386/kernel/nmi.c
> index a98ba88..9f1e8c1 100644
> --- a/arch/i386/kernel/nmi.c
> +++ b/arch/i386/kernel/nmi.c
> @@ -41,16 +41,17 @@ int nmi_watchdog_enabled;
>   *   different subsystems this reservation system just tries to coordinate
>   *   things a little
>   */
> -static DEFINE_PER_CPU(unsigned long, perfctr_nmi_owner);
> -static DEFINE_PER_CPU(unsigned long, evntsel_nmi_owner[3]);
> -
> -static cpumask_t backtrace_mask = CPU_MASK_NONE;
>  
>  /* this number is calculated from Intel's MSR_P4_CRU_ESCR5 register and it's
>   * offset from MSR_P4_BSU_ESCR0.  It will be the max for all platforms (for now)
>   */
>  #define NMI_MAX_COUNTER_BITS 66
> +#define NMI_MAX_COUNTER_LONGS BITS_TO_LONGS(NMI_MAX_COUNTER_BITS)
>  
> +static DEFINE_PER_CPU(unsigned long, perfctr_nmi_owner[NMI_MAX_COUNTER_LONGS]);
> +static DEFINE_PER_CPU(unsigned long, evntsel_nmi_owner[NMI_MAX_COUNTER_LONGS]);
> +
> +static cpumask_t backtrace_mask = CPU_MASK_NONE;
>  /* nmi_active:
>   * >0: the lapic NMI watchdog is active, but can be disabled
>   * <0: the lapic NMI watchdog has not been set up, and cannot

The created a warning storm:


arch/i386/kernel/nmi.c: In function 'avail_to_resrv_perfctr_nmi_bit':
arch/i386/kernel/nmi.c:129: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type
arch/i386/kernel/nmi.c:129: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type
arch/i386/kernel/nmi.c: In function 'avail_to_resrv_perfctr_nmi':
arch/i386/kernel/nmi.c:145: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type
arch/i386/kernel/nmi.c:145: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type
arch/i386/kernel/nmi.c: In function '__reserve_perfctr_nmi':
arch/i386/kernel/nmi.c:160: warning: passing argument 2 of 'test_and_set_bit' from incompatible pointer type
arch/i386/kernel/nmi.c: In function '__release_perfctr_nmi':
arch/i386/kernel/nmi.c:174: warning: passing argument 2 of 'clear_bit' from incompatible pointer type

diff -puN arch/i386/kernel/nmi.c~fix-x86-fix-potential-overflow-in-perfctr-reservation arch/i386/kernel/nmi.c
--- a/arch/i386/kernel/nmi.c~fix-x86-fix-potential-overflow-in-perfctr-reservation
+++ a/arch/i386/kernel/nmi.c
@@ -126,7 +126,7 @@ int avail_to_resrv_perfctr_nmi_bit(unsig
 	int cpu;
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 	for_each_possible_cpu (cpu) {
-		if (test_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+		if (test_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 			return 0;
 	}
 	return 1;
@@ -142,7 +142,7 @@ int avail_to_resrv_perfctr_nmi(unsigned 
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
 	for_each_possible_cpu (cpu) {
-		if (test_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+		if (test_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 			return 0;
 	}
 	return 1;
@@ -157,7 +157,7 @@ static int __reserve_perfctr_nmi(int cpu
 	counter = nmi_perfctr_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	if (!test_and_set_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+	if (!test_and_set_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 		return 1;
 	return 0;
 }
@@ -171,7 +171,7 @@ static void __release_perfctr_nmi(int cp
 	counter = nmi_perfctr_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	clear_bit(counter, &per_cpu(perfctr_nmi_owner, cpu));
+	clear_bit(counter, per_cpu(perfctr_nmi_owner, cpu));
 }
 
 int reserve_perfctr_nmi(unsigned int msr)
_


I worry rather a lot about how well runtime tested this very late change
was, and whether it works correctly even with this fix applied.  Perhaps
we should jsut revert?

Re: [PATCH] x86: Fix potential overflow in perfctr reservation

[Andi Kleen]

> 
> The created a warning storm:

Hmm, yes good idea to fix that. Probably for x86-64 too.

> 
> I worry rather a lot about how well runtime tested this very late change
> was, 

I tested it with oprofile and checked the nmi watchdog.

> and whether it works correctly even with this fix applied.  Perhaps 
> we should jsut revert?

Then you get the memory corruption back.

The change really only enlarges the bitmaps a little anyways, it does not
change any algorithms.

-Andi
 
 

Re: [PATCH] x86: Fix potential overflow in perfctr reservation

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <20070422010917.6427e7a2.akpm@linux-foundation.org> (at Sun, 22 Apr 2007 01:09:17 -0700), Andrew Morton <akpm@linux-foundation.org> says:

> >     [PATCH] x86: Fix potential overflow in perfctr reservation
:
> The created a warning storm:
> 
> 
> arch/i386/kernel/nmi.c: In function 'avail_to_resrv_perfctr_nmi_bit':
> arch/i386/kernel/nmi.c:129: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type
> arch/i386/kernel/nmi.c:129: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type
:
> diff -puN arch/i386/kernel/nmi.c~fix-x86-fix-potential-overflow-in-perfctr-reservation arch/i386/kernel/nmi.c
> --- a/arch/i386/kernel/nmi.c~fix-x86-fix-potential-overflow-in-perfctr-reservation
> +++ a/arch/i386/kernel/nmi.c
> @@ -126,7 +126,7 @@ int avail_to_resrv_perfctr_nmi_bit(unsig
>  	int cpu;
>  	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
>  	for_each_possible_cpu (cpu) {
> -		if (test_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
> +		if (test_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
>  			return 0;
>  	}
>  	return 1;
:
> 
> I worry rather a lot about how well runtime tested this very late change
> was, and whether it works correctly even with this fix applied.  Perhaps
> we should jsut revert?

Is DEFINE_PER_CPU(type, var[num]) is really valid?
I guess it should be DEFINE_PER_CPU(type[num], var), no?

----
[I386] NMI: Fix per_cpu() usage.

Per-cpu array should be declared as DEFINE_PER_CPU(type[size], name),
not as DEFINE_PER_CPU(type, name[size]).

Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

diff --git a/arch/i386/kernel/nmi.c b/arch/i386/kernel/nmi.c
index 9f1e8c1..eddb4f7 100644
--- a/arch/i386/kernel/nmi.c
+++ b/arch/i386/kernel/nmi.c
@@ -48,8 +48,8 @@ int nmi_watchdog_enabled;
 #define NMI_MAX_COUNTER_BITS 66
 #define NMI_MAX_COUNTER_LONGS BITS_TO_LONGS(NMI_MAX_COUNTER_BITS)
 
-static DEFINE_PER_CPU(unsigned long, perfctr_nmi_owner[NMI_MAX_COUNTER_LONGS]);
-static DEFINE_PER_CPU(unsigned long, evntsel_nmi_owner[NMI_MAX_COUNTER_LONGS]);
+static DEFINE_PER_CPU(unsigned long [NMI_MAX_COUNTER_LONGS], perfctr_nmi_owner);
+static DEFINE_PER_CPU(unsigned long [NMI_MAX_COUNTER_LONGS], evntsel_nmi_owner);
 
 static cpumask_t backtrace_mask = CPU_MASK_NONE;
 /* nmi_active:
@@ -126,7 +126,7 @@ int avail_to_resrv_perfctr_nmi_bit(unsigned int counter)
 	int cpu;
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 	for_each_possible_cpu (cpu) {
-		if (test_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+		if (test_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 			return 0;
 	}
 	return 1;
@@ -142,7 +142,7 @@ int avail_to_resrv_perfctr_nmi(unsigned int msr)
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
 	for_each_possible_cpu (cpu) {
-		if (test_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+		if (test_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 			return 0;
 	}
 	return 1;
@@ -157,7 +157,7 @@ static int __reserve_perfctr_nmi(int cpu, unsigned int msr)
 	counter = nmi_perfctr_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	if (!test_and_set_bit(counter, &per_cpu(perfctr_nmi_owner, cpu)))
+	if (!test_and_set_bit(counter, per_cpu(perfctr_nmi_owner, cpu)))
 		return 1;
 	return 0;
 }
@@ -171,7 +171,7 @@ static void __release_perfctr_nmi(int cpu, unsigned int msr)
 	counter = nmi_perfctr_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	clear_bit(counter, &per_cpu(perfctr_nmi_owner, cpu));
+	clear_bit(counter, per_cpu(perfctr_nmi_owner, cpu));
 }
 
 int reserve_perfctr_nmi(unsigned int msr)
@@ -207,7 +207,7 @@ int __reserve_evntsel_nmi(int cpu, unsigned int msr)
 	counter = nmi_evntsel_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	if (!test_and_set_bit(counter, &per_cpu(evntsel_nmi_owner, cpu)[0]))
+	if (!test_and_set_bit(counter, per_cpu(evntsel_nmi_owner, cpu)))
 		return 1;
 	return 0;
 }
@@ -221,7 +221,7 @@ static void __release_evntsel_nmi(int cpu, unsigned int msr)
 	counter = nmi_evntsel_msr_to_bit(msr);
 	BUG_ON(counter > NMI_MAX_COUNTER_BITS);
 
-	clear_bit(counter, &per_cpu(evntsel_nmi_owner, cpu)[0]);
+	clear_bit(counter, per_cpu(evntsel_nmi_owner, cpu));
 }
 
 int reserve_evntsel_nmi(unsigned int msr)

-- 
YOSHIFUJI Hideaki @ USAGI Project  <yoshfuji@linux-ipv6.org>
GPG-FP  : 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA