linux-kernel - Re: Entry conditions for perf_event_do

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 13 Jan 2010 10:27:39 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Paul Mackerras <paulus@...ba.org>
Cc:	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	Milton Miller <miltonm@....com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: Entry conditions for perf_event_do_pending?

On Wed, 2010-01-13 at 15:14 +1100, Paul Mackerras wrote:
> We're seeing some perf-related crashes on powerpc related to having
> irqs in an inconsistent state (soft-enable vs. hard-enable
> vs. trace-irqs state) when entering perf_event_do_pending().
> We're fixing that, but along the way we have struck a question about
> what conditions are required on entry to perf_event_do_pending.
> 
> Its use of __get_cpu_var implies that it at least needs to be called
> with either interrupts or preemption disabled.  Does it in fact need
> to be called with irqs off?  Do we need to call irq_enter()/irq_exit()
> around it?  Are there any other requirements that people can think of?

Right, so when I wrote it all it required was preempt disabled, but then
I added all that disable stuff (perf_pending_event():
event->pending_disable) and that requires IRQs disabled because it calls
__perf_event_disable() which takes ctx->lock, which is supposed to be an
IRQ safe lock.

On x86 we always run it from a self-ipi, which is I guess why the
generic timer softirq callback never triggered for me, because that
looks broken.

So in short, I think perf_event_do_pending() requires full IRQ context,
if that includes calling irq_enter()/irq_exit() then yes.

Something like the below ought to do I guess..

---
 kernel/timer.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 15533b7..c61a794 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1198,6 +1198,7 @@ void update_process_times(int user_tick)
 	run_local_timers();
 	rcu_check_callbacks(cpu, user_tick);
 	printk_tick();
+	perf_event_do_pending();
 	scheduler_tick();
 	run_posix_cpu_timers(p);
 }
@@ -1209,8 +1210,6 @@ static void run_timer_softirq(struct softirq_action *h)
 {
 	struct tvec_base *base = __get_cpu_var(tvec_bases);
 
-	perf_event_do_pending();
-
 	hrtimer_run_pending();
 
 	if (time_after_eq(jiffies, base->timer_jiffies))


--
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/