| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 06 Apr 2009 01:44:55 +0530
From: Jaswinder Singh Rajput <jaswinder@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: John Levon <levon@...ementarian.org>, Ingo Molnar <mingo@...e.hu>,
x86 maintainers <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: fix BUG: using smp_processor_id() in preemptible
[00000000] code: oprofiled/3319
On Sun, 2009-04-05 at 12:51 -0700, Linus Torvalds wrote:
>
> On Mon, 6 Apr 2009, Jaswinder Singh Rajput wrote:
> >
> > Fixed this bug on P4 HT machine:
>
> I don't think this really is correct.
>
> Anything that does that "get_stagger()" thing should already either be
> tied to a specific CPU (because it's going to actually touch the variables
> for "that CPU"), or it should likely set up the data structures for _all_
> cases.
>
> Just as an example, look at something like "p4_setup_ctrs()": it will use
> that stagger to determine what MSR to read. But if we are preemptable, the
> CPU we actually do the MSR read on may be a _different_ CPU than the CPU
> that we did the "get_stagger()" on!
>
> So I really think that protecting against preemption inside of
> "get_stagger()" is fundamentally buggy. Because if you need it, then by
> the time you return the value from the function, the value has now lost
> all meaning because you might have preempted to another CPU after doing
> the put_cpu().
>
> So I think the "get_cpu()/put_cpu()" should be done around the whole
> sequence (ie from before "get_stagger()" to after the stagger has been
> used to initialize some data structures or do wrmsr/rdmsr calls.
>
> Of course, this is all ancient code, so whatever. But I really think this
> patch is actively bad - it just hides the issue rather than fixing
> anything.
>
Subject: [PATCH] x86: fix BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/3319
Fixed this bug on P4 HT machine:
[ 474.968698] BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/3319
[ 474.968711] caller is get_stagger+0xe/0x2c
[ 474.968716] Pid: 3319, comm: oprofiled Not tainted 2.6.29 #8
[ 474.968720] Call Trace:
[ 474.968730] [<c03dee3a>] ? printk+0x14/0x16
[ 474.968737] [<c021bcb8>] debug_smp_processor_id+0xa4/0xb8
[ 474.968742] [<c034d64d>] get_stagger+0xe/0x2c
[ 474.968747] [<c034daa7>] p4_fill_in_addresses+0x33/0x2d4
[ 474.968751] [<c034cc06>] nmi_setup+0xce/0x194
[ 474.968756] [<c034acf9>] oprofile_setup+0x35/0x90
[ 474.968760] [<c034bc55>] event_buffer_open+0x47/0x63
[ 474.968766] [<c0179a8d>] __dentry_open+0x148/0x237
[ 474.968770] [<c0179c20>] nameidata_to_filp+0x31/0x48
[ 474.968775] [<c034bc0e>] ? event_buffer_open+0x0/0x63
[ 474.968780] [<c018493e>] do_filp_open+0x335/0x660
[ 474.968786] [<c0161f85>] ? lru_cache_add_lru+0x2c/0x2e
[ 474.968792] [<c0171808>] ? page_add_new_anon_rmap+0x5d/0x6a
[ 474.968797] [<c017985d>] do_sys_open+0x47/0xc1
[ 474.968801] [<c0179923>] sys_open+0x23/0x2b
[ 474.968806] [<c01029f4>] sysenter_do_call+0x12/0x26
Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@...il.com>
---
arch/x86/oprofile/op_model_p4.c | 17 ++++++++++++++++-
1 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/arch/x86/oprofile/op_model_p4.c b/arch/x86/oprofile/op_model_p4.c
index 4c4a51c..8db034d 100644
--- a/arch/x86/oprofile/op_model_p4.c
+++ b/arch/x86/oprofile/op_model_p4.c
@@ -379,12 +379,18 @@ static struct p4_event_binding p4_events[NUM_EVENTS] = {
static unsigned int get_stagger(void)
{
#ifdef CONFIG_SMP
- int cpu = smp_processor_id();
+ int cpu = get_cpu();
return (cpu != first_cpu(per_cpu(cpu_sibling_map, cpu)));
#endif
return 0;
}
+static inline void put_stagger(void)
+{
+#ifdef CONFIG_SMP
+ put_cpu();
+#endif
+}
/* finally, mediate access to a real hardware counter
by passing a "virtual" counter numer to this macro,
@@ -482,6 +488,7 @@ static void p4_fill_in_addresses(struct op_msrs * const msrs)
msrs->controls[i++].addr = MSR_P4_CRU_ESCR5;
}
}
+ put_stagger();
}
@@ -506,6 +513,7 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
printk(KERN_ERR
"oprofile: P4 event code 0x%lx out of range\n",
counter_config[ctr].event);
+ put_stagger();
return;
}
@@ -538,6 +546,7 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
else
CCCR_SET_PMI_OVF_1(cccr);
CCCR_WRITE(cccr, high, VIRT_CTR(stag, ctr));
+ put_stagger();
return;
}
}
@@ -545,6 +554,7 @@ static void pmc_setup_one_p4_counter(unsigned int ctr)
printk(KERN_ERR
"oprofile: P4 event code 0x%lx no binding, stag %d ctr %d\n",
counter_config[ctr].event, stag, ctr);
+ put_stagger();
}
@@ -559,6 +569,7 @@ static void p4_setup_ctrs(struct op_msrs const * const msrs)
rdmsr(MSR_IA32_MISC_ENABLE, low, high);
if (!MISC_PMC_ENABLED_P(low)) {
printk(KERN_ERR "oprofile: P4 PMC not available\n");
+ put_stagger();
return;
}
@@ -589,6 +600,7 @@ static void p4_setup_ctrs(struct op_msrs const * const msrs)
reset_value[i] = 0;
}
}
+ put_stagger();
}
@@ -638,6 +650,7 @@ static int p4_check_ctrs(struct pt_regs * const regs,
/* P4 quirk: you have to re-unmask the apic vector */
apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
+ put_stagger();
/* See op_model_ppro.c */
return 1;
}
@@ -657,6 +670,7 @@ static void p4_start(struct op_msrs const * const msrs)
CCCR_SET_ENABLE(low);
CCCR_WRITE(low, high, VIRT_CTR(stag, i));
}
+ put_stagger();
}
@@ -674,6 +688,7 @@ static void p4_stop(struct op_msrs const * const msrs)
CCCR_SET_DISABLE(low);
CCCR_WRITE(low, high, VIRT_CTR(stag, i));
}
+ put_stagger();
}
static void p4_shutdown(struct op_msrs const * const msrs)
--
1.6.0.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists