netdev - [PATCH] hrtimer, softirq: Fix hrtimer->softirq trampoline

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:	Tue, 02 Feb 2010 15:20:01 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Yury Polyanskiy <ypolyans@...nceton.edu>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	Wei Yongjun <yjwei@...fujitsu.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>, polyanskiy@...il.com,
	Thomas Gleixner <tglx@...utronix.de>,
	lkml <linux-kernel@...r.kernel.org>
Subject: [PATCH]  hrtimer, softirq: Fix hrtimer->softirq trampoline

On Tue, 2010-02-02 at 08:51 -0500, Yury Polyanskiy wrote:

> If hrtimer_tasklet interface functions properly, the
> xfrm_timer_handler should be called in softirq context (and thus is
> never in parallel with xfrm_input()). The deadlock isn't possible then.
> 
> In this case it seems that for some reason xfrm_timer_handler() is
> called in the hardirq context. The relevant code in hrtimer_tasklet:
> 
> static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
> {
> 	struct tasklet_hrtimer *ttimer =
> 		container_of(timer, struct tasklet_hrtimer, timer);
> 
> 	if (hrtimer_is_hres_active(timer)) {
> 		tasklet_hi_schedule(&ttimer->tasklet);
> 		return HRTIMER_NORESTART;
> 	}
> 	return ttimer->function(timer);
> }
> 
> I am copying Peter on this. Peter, how is it possible that
> ttimer->function() is called in hardirq?
> 
> Could it be that switch from hres_active happened after the call to
> trampoline and before the if() above?

The original email had more information:

> {IN-HARDIRQ-W} state was registered at:
>   [<c04718dc>] __lock_acquire+0xa9c/0x1890
>   [<c047274f>] lock_acquire+0x7f/0xf0
>   [<c0762958>] _raw_spin_lock+0x38/0x50
>   [<c072b5ca>] xfrm_timer_handler+0x3a/0x260
>   [<c0447d9d>] __hrtimer_tasklet_trampoline+0xd/0x10
>   [<c04634ce>] hrtimer_run_queues+0x15e/0x2a0
>   [<c045146d>] run_local_timers+0xd/0x20
>   [<c04514b4>] update_process_times+0x34/0x70
>   [<c046ce8a>] tick_periodic+0x2a/0x80
>   [<c046cefe>] tick_handle_periodic+0x1e/0x90
>   [<c0768377>] smp_apic_timer_interrupt+0x57/0x8b
>   [<c076382f>] apic_timer_interrupt+0x2f/0x34
>   [<c0401d3b>] cpu_idle+0x4b/0x80
>   [<c074e0d7>] rest_init+0x67/0x70
>   [<c0956874>] start_kernel+0x30e/0x314
>   [<c095609e>] i386_start_kernel+0x9e/0xa5

Which indicates we were called from hardirq context, it appears that
that hrtimer_is_hres_active() case is indeed faulty. Not sure if I made
a mistake when I wrote that or if we changed hrtimer behaviour
afterwards, but the hrtimer fallback is still from hardirq context.

Which would seem to suggest the following patch:

---
Subject: hrtimer, softirq: Fix hrtimer->softirq trampoline

hrtimers callbacks are always done from hardirq context, either the
jiffy tick interrupt or the hrtimer device interrupt.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
---
 kernel/softirq.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index a09502e..c1983b7 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -500,22 +500,15 @@ EXPORT_SYMBOL(tasklet_kill);
  */
 
 /*
- * The trampoline is called when the hrtimer expires. If this is
- * called from the hrtimer interrupt then we schedule the tasklet as
- * the timer callback function expects to run in softirq context. If
- * it's called in softirq context anyway (i.e. high resolution timers
- * disabled) then the hrtimer callback is called right away.
+ * The trampoline is called when the hrtimer expires. 
  */
 static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
 {
 	struct tasklet_hrtimer *ttimer =
 		container_of(timer, struct tasklet_hrtimer, timer);
 
-	if (hrtimer_is_hres_active(timer)) {
-		tasklet_hi_schedule(&ttimer->tasklet);
-		return HRTIMER_NORESTART;
-	}
-	return ttimer->function(timer);
+	tasklet_hi_schedule(&ttimer->tasklet);
+	return HRTIMER_NORESTART;
 }
 
 /*


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html