* Re: [PATCH] x86: Fix potential overflow in perfctr reservation [not found] <200704172359.l3HNxAMV024586@hera.kernel.org> @ 2007-04-22 8:09 ` Andrew Morton 2007-04-22 12:14 ` Andi Kleen 2007-04-23 5:11 ` YOSHIFUJI Hideaki / 吉藤英明 0 siblings, 2 replies; 3+ messages in thread From: Andrew Morton @ 2007-04-22 8:09 UTC (permalink / raw) To: Andi Kleen; +Cc: Linux Kernel Mailing List 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? ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: Fix potential overflow in perfctr reservation 2007-04-22 8:09 ` [PATCH] x86: Fix potential overflow in perfctr reservation Andrew Morton @ 2007-04-22 12:14 ` Andi Kleen 2007-04-23 5:11 ` YOSHIFUJI Hideaki / 吉藤英明 1 sibling, 0 replies; 3+ messages in thread From: Andi Kleen @ 2007-04-22 12:14 UTC (permalink / raw) To: Andrew Morton; +Cc: Linux Kernel Mailing List > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: Fix potential overflow in perfctr reservation 2007-04-22 8:09 ` [PATCH] x86: Fix potential overflow in perfctr reservation Andrew Morton 2007-04-22 12:14 ` Andi Kleen @ 2007-04-23 5:11 ` YOSHIFUJI Hideaki / 吉藤英明 1 sibling, 0 replies; 3+ messages in thread From: YOSHIFUJI Hideaki / 吉藤英明 @ 2007-04-23 5:11 UTC (permalink / raw) To: akpm; +Cc: ak, linux-kernel, yoshfuji 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-04-23 5:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200704172359.l3HNxAMV024586@hera.kernel.org>
2007-04-22 8:09 ` [PATCH] x86: Fix potential overflow in perfctr reservation Andrew Morton
2007-04-22 12:14 ` Andi Kleen
2007-04-23 5:11 ` YOSHIFUJI Hideaki / 吉藤英明
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox