linux-kernel - Re: linux-next: Tree for Feb 4

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]
Message-ID: <20150205181141.6f1d532a@gandalf.local.home>
Date:	Thu, 5 Feb 2015 18:11:41 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Sedat Dilek <sedat.dilek@...il.com>
Cc:	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Dave Hansen <dave@...1.net>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	linux-next <linux-next@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Kristen Carlson Accardi <kristen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>, Mel Gorman <mgorman@...e.de>
Subject: Re: linux-next: Tree for Feb 4

On Thu, 5 Feb 2015 23:16:21 +0100
Sedat Dilek <sedat.dilek@...il.com> wrote:

> On Thu, Feb 5, 2015 at 11:09 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> > On Thu, 5 Feb 2015 22:45:59 +0100
> > Sedat Dilek <sedat.dilek@...il.com> wrote:
> >
> >> Steve, this was a typo it's called tlb_flush not tlb_flush*ed*:
> >
> > Heh, yeah, I typed that entire line in by hand. Just be lucky that was
> > the only typo ;-)
> >
> >>
> >> # cat /sys/kernel/debug/tracing/events/tlb/tlb_flush/enable
> >> 1
> >>
> >> [  391.090381] intel_pstate CPU 1 exiting
> >> [  391.104491] smpboot: CPU 1 is now offline
> >>
> >
> > Now, if you disable that (echo 0 to that file), do you still get the
> > rcu lockdep splat if you suspend and resume?
> >
> 
> YES, I get the call-trace again!
> 

Bah! I see where the warning comes from. In include/linux/tracepoint.h
we have:

#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
	extern struct tracepoint __tracepoint_##name;			\
	static inline void trace_##name(proto)				\
	{								\
		if (static_key_false(&__tracepoint_##name.key))		\
			__DO_TRACE(&__tracepoint_##name,		\
				TP_PROTO(data_proto),			\
				TP_ARGS(data_args),			\
				TP_CONDITION(cond),,);			\
		if (IS_ENABLED(CONFIG_LOCKDEP)) {			\
			rcu_read_lock_sched_notrace();			\
			rcu_dereference_sched(__tracepoint_##name.funcs);\
			rcu_read_unlock_sched_notrace();		\
		}							\
	}								\

See that if (IS_ENABLED(CONFIG_LOCKDEP))?

I'm recalling this. Because tracepoints require RCU, and RCU lockdep
doesn't trigger if a tracepoint isn't enabled (because the rcu calls
are hidden in the __DO_TRACE() behind that static_key_false), we would
be missing lots of rcu problem tracepoints because tests were run
without them enabled.

The answer was to add this rcu check when LOCKDEP was enabled. So no,
adding that conditional isn't going to help, because lockdep will
trigger here, even if it were safe because of the conditional :-/.

That said, let's add this (on top of the old patch):

(again, not tested)

Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
-------
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 4b75d591eb5e..401b5bfbcdbd 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -47,7 +47,12 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
 
 		/* Re-load page tables */
 		load_cr3(next->pgd);
-		trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+		/*
+		 * Do not check rcu when tracing is not enabled. The
+		 * tracepoint has a condition to not trace if the CPU is
+		 * offline, and rcu check will complain if it is.
+		 */
+		trace_tlb_flush_rcu_nocheck(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
 
 		/* Stop flush ipis for the previous mm */
 		cpumask_clear_cpu(cpu, mm_cpumask(prev));
@@ -84,7 +89,13 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
 			 * to make sure to use no freed page tables.
 			 */
 			load_cr3(next->pgd);
-			trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+			/*
+			 * Do not check rcu when tracing is not enabled. The
+			 * tracepoint has a condition to not trace if the CPU is
+			 * offline, and rcu check will complain if it is.
+			 */
+			trace_tlb_flush_rcu_nocheck(TLB_FLUSH_ON_TASK_SWITCH,
+						    TLB_FLUSH_ALL);
 			load_LDT_nolock(&next->context);
 		}
 	}
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index e08e21e5f601..747a05aceb60 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -179,6 +179,14 @@ extern void syscall_unregfunc(void);
 			rcu_read_unlock_sched_notrace();		\
 		}							\
 	}								\
+	static inline void trace_##name##_rcu_nocheck(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),,);			\
+	}								\
 	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
 		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
 	static inline int						\
@@ -230,6 +238,8 @@ extern void syscall_unregfunc(void);
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
 	static inline void trace_##name(proto)				\
 	{ }								\
+	static inline void trace_##name##_rcu_nocheck(proto)		\
+	{ }								\
 	static inline void trace_##name##_rcuidle(proto)		\
 	{ }								\
 	static inline int						\

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