linux-kernel - Re: [PATCH 0/1] stop the unbound recursion in preempt_schedule

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:	Mon, 6 Oct 2014 01:53:35 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>
Cc:	Sasha Levin <sasha.levin@...cle.com>,
	Ingo Molnar <mingo@...nel.org>, Peter Anvin <hpa@...or.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...capital.net>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Chuck Ebbert <cebbert.lkml@...il.com>
Subject: Re: [PATCH 0/1] stop the unbound recursion in
	preempt_schedule_context()

And, Frederic, this is off-topic but I am just curious...

I can't understand why exception_enter() calls context_tracking_user_exit()
unconditionally (unlike exception_exit), and why enter/exit need to check
context_tracking.state with irqs disabled.

IOW, any reason why the patch below is wrong?

Of course, context_tracking is per-cpu, so

	if (context_tracking_in_user()) {
		local_irq_save(flags);
		...

can be preempted and change the state on another CPU. But this should
be fine because the task must see the same .state on all CPU's or the
whole preempt context-tracking logic is broken?

Oleg.


--- x/include/linux/context_tracking.h
+++ x/include/linux/context_tracking.h
@@ -29,15 +29,14 @@ static inline void user_exit(void)
 
 static inline enum ctx_state exception_enter(void)
 {
-	enum ctx_state prev_ctx;
-
-	if (!context_tracking_is_enabled())
-		return 0;
-
-	prev_ctx = this_cpu_read(context_tracking.state);
-	context_tracking_user_exit();
+	if (context_tracking_is_enabled()) {
+		if (context_tracking_in_user()) {
+			context_tracking_user_exit();
+			return IN_USER;
+		}
+	}
 
-	return prev_ctx;
+	return IN_KERNEL;
 }
 
 static inline void exception_exit(enum ctx_state prev_ctx)
--- x/kernel/context_tracking.c
+++ x/kernel/context_tracking.c
@@ -74,8 +74,8 @@ void context_tracking_user_enter(void)
 	/* Kernel threads aren't supposed to go to userspace */
 	WARN_ON_ONCE(!current->mm);
 
-	local_irq_save(flags);
-	if ( __this_cpu_read(context_tracking.state) != IN_USER) {
+	if (!context_tracking_in_user()) {
+		local_irq_save(flags);
 		if (__this_cpu_read(context_tracking.active)) {
 			trace_user_enter(0);
 			/*
@@ -102,8 +102,8 @@ void context_tracking_user_enter(void)
 		 * is false because we know that CPU is not tickless.
 		 */
 		__this_cpu_write(context_tracking.state, IN_USER);
+		local_irq_restore(flags);
 	}
-	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(context_tracking_user_enter);
 
@@ -128,8 +128,8 @@ void context_tracking_user_exit(void)
 	if (in_interrupt())
 		return;
 
-	local_irq_save(flags);
-	if (__this_cpu_read(context_tracking.state) == IN_USER) {
+	if (context_tracking_in_user()) {
+		local_irq_save(flags);
 		if (__this_cpu_read(context_tracking.active)) {
 			/*
 			 * We are going to run code that may use RCU. Inform
@@ -140,8 +140,8 @@ void context_tracking_user_exit(void)
 			trace_user_exit(0);
 		}
 		__this_cpu_write(context_tracking.state, IN_KERNEL);
+		local_irq_restore(flags);
 	}
-	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(context_tracking_user_exit);
 

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