[for-linus][PATCH 0/5] tracing: Fixes for v6.3

[for-linus][PATCH 0/5] tracing: Fixes for v6.3

[Steven Rostedt]

Arnd Bergmann (1):
      ftrace,kcfi: Define ftrace_stub_graph conditionally

Chen Zhongjin (1):
      ftrace: Fix invalid address access in lookup_rec() when index is 0

Steven Rostedt (Google) (3):
      tracing: Do not let histogram values have some modifiers
      tracing: Check field value in hist_field_name()
      tracing: Make tracepoint lockdep check actually test something

----
 arch/x86/kernel/ftrace_64.S      |  2 ++
 include/linux/tracepoint.h       | 15 ++++++---------
 kernel/trace/ftrace.c            |  3 ++-
 kernel/trace/trace_events_hist.c | 12 ++++++++++++
 4 files changed, 22 insertions(+), 10 deletions(-)

[for-linus][PATCH 1/5] tracing: Do not let histogram values have some modifiers

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Histogram values can not be strings, stacktraces, graphs, symbols,
syscalls, or grouped in buckets or log. Give an error if a value is set to
do so.

Note, the histogram code was not prepared to handle these modifiers for
histograms and caused a bug.

Mark Rutland reported:

 # echo 'p:copy_to_user __arch_copy_to_user n=$arg2' >> /sys/kernel/tracing/kprobe_events
 # echo 'hist:keys=n:vals=hitcount.buckets=8:sort=hitcount' > /sys/kernel/tracing/events/kprobes/copy_to_user/trigger
 # cat /sys/kernel/tracing/events/kprobes/copy_to_user/hist
[  143.694628] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  143.695190] Mem abort info:
[  143.695362]   ESR = 0x0000000096000004
[  143.695604]   EC = 0x25: DABT (current EL), IL = 32 bits
[  143.695889]   SET = 0, FnV = 0
[  143.696077]   EA = 0, S1PTW = 0
[  143.696302]   FSC = 0x04: level 0 translation fault
[  143.702381] Data abort info:
[  143.702614]   ISV = 0, ISS = 0x00000004
[  143.702832]   CM = 0, WnR = 0
[  143.703087] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000448f9000
[  143.703407] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[  143.704137] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[  143.704714] Modules linked in:
[  143.705273] CPU: 0 PID: 133 Comm: cat Not tainted 6.2.0-00003-g6fc512c10a7c #3
[  143.706138] Hardware name: linux,dummy-virt (DT)
[  143.706723] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  143.707120] pc : hist_field_name.part.0+0x14/0x140
[  143.707504] lr : hist_field_name.part.0+0x104/0x140
[  143.707774] sp : ffff800008333a30
[  143.707952] x29: ffff800008333a30 x28: 0000000000000001 x27: 0000000000400cc0
[  143.708429] x26: ffffd7a653b20260 x25: 0000000000000000 x24: ffff10d303ee5800
[  143.708776] x23: ffffd7a6539b27b0 x22: ffff10d303fb8c00 x21: 0000000000000001
[  143.709127] x20: ffff10d303ec2000 x19: 0000000000000000 x18: 0000000000000000
[  143.709478] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[  143.709824] x14: 0000000000000000 x13: 203a6f666e692072 x12: 6567676972742023
[  143.710179] x11: 0a230a6d6172676f x10: 000000000000002c x9 : ffffd7a6521e018c
[  143.710584] x8 : 000000000000002c x7 : 7f7f7f7f7f7f7f7f x6 : 000000000000002c
[  143.710915] x5 : ffff10d303b0103e x4 : ffffd7a653b20261 x3 : 000000000000003d
[  143.711239] x2 : 0000000000020001 x1 : 0000000000000001 x0 : 0000000000000000
[  143.711746] Call trace:
[  143.712115]  hist_field_name.part.0+0x14/0x140
[  143.712642]  hist_field_name.part.0+0x104/0x140
[  143.712925]  hist_field_print+0x28/0x140
[  143.713125]  event_hist_trigger_print+0x174/0x4d0
[  143.713348]  hist_show+0xf8/0x980
[  143.713521]  seq_read_iter+0x1bc/0x4b0
[  143.713711]  seq_read+0x8c/0xc4
[  143.713876]  vfs_read+0xc8/0x2a4
[  143.714043]  ksys_read+0x70/0xfc
[  143.714218]  __arm64_sys_read+0x24/0x30
[  143.714400]  invoke_syscall+0x50/0x120
[  143.714587]  el0_svc_common.constprop.0+0x4c/0x100
[  143.714807]  do_el0_svc+0x44/0xd0
[  143.714970]  el0_svc+0x2c/0x84
[  143.715134]  el0t_64_sync_handler+0xbc/0x140
[  143.715334]  el0t_64_sync+0x190/0x194
[  143.715742] Code: a9bd7bfd 910003fd a90153f3 aa0003f3 (f9400000)
[  143.716510] ---[ end trace 0000000000000000 ]---
Segmentation fault

Link: https://lkml.kernel.org/r/20230302020810.559462599@goodmis.org

Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 89877a18f933..6e8ab726a7b5 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4235,6 +4235,15 @@ static int __create_val_field(struct hist_trigger_data *hist_data,
 		goto out;
 	}
 
+	/* Some types cannot be a value */
+	if (hist_field->flags & (HIST_FIELD_FL_GRAPH | HIST_FIELD_FL_PERCENT |
+				 HIST_FIELD_FL_BUCKET | HIST_FIELD_FL_LOG2 |
+				 HIST_FIELD_FL_SYM | HIST_FIELD_FL_SYM_OFFSET |
+				 HIST_FIELD_FL_SYSCALL | HIST_FIELD_FL_STACKTRACE)) {
+		hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
+		ret = -EINVAL;
+	}
+
 	hist_data->fields[val_idx] = hist_field;
 
 	++hist_data->n_vals;
-- 
2.39.1

[for-linus][PATCH 2/5] tracing: Check field value in hist_field_name()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The function hist_field_name() cannot handle being passed a NULL field
parameter. It should never be NULL, but due to a previous bug, NULL was
passed to the function and the kernel crashed due to a NULL dereference.
Mark Rutland reported this to me on IRC.

The bug was fixed, but to prevent future bugs from crashing the kernel,
check the field and add a WARN_ON() if it is NULL.

Link: https://lkml.kernel.org/r/20230302020810.762384440@goodmis.org

Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Reported-by: Mark Rutland <mark.rutland@arm.com>
Fixes: c6afad49d127f ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6e8ab726a7b5..486cca3c2b75 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1331,6 +1331,9 @@ static const char *hist_field_name(struct hist_field *field,
 {
 	const char *field_name = "";
 
+	if (WARN_ON_ONCE(!field))
+		return field_name;
+
 	if (level > 1)
 		return field_name;
 
-- 
2.39.1

[for-linus][PATCH 3/5] ftrace: Fix invalid address access in lookup_rec() when index is 0

[Steven Rostedt]

From: Chen Zhongjin <chenzhongjin@huawei.com>

KASAN reported follow problem:

 BUG: KASAN: use-after-free in lookup_rec
 Read of size 8 at addr ffff000199270ff0 by task modprobe
 CPU: 2 Comm: modprobe
 Call trace:
  kasan_report
  __asan_load8
  lookup_rec
  ftrace_location
  arch_check_ftrace_location
  check_kprobe_address_safe
  register_kprobe

When checking pg->records[pg->index - 1].ip in lookup_rec(), it can get a
pg which is newly added to ftrace_pages_start in ftrace_process_locs().
Before the first pg->index++, index is 0 and accessing pg->records[-1].ip
will cause this problem.

Don't check the ip when pg->index is 0.

Link: https://lore.kernel.org/linux-trace-kernel/20230309080230.36064-1-chenzhongjin@huawei.com

Cc: stable@vger.kernel.org
Fixes: 9644302e3315 ("ftrace: Speed up search by skipping pages by address")
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 750aa3f08b25..a47f7d93e32d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1537,7 +1537,8 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
 	key.flags = end;	/* overload flags, as it is unsigned long */
 
 	for (pg = ftrace_pages_start; pg; pg = pg->next) {
-		if (end < pg->records[0].ip ||
+		if (pg->index == 0 ||
+		    end < pg->records[0].ip ||
 		    start >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
 			continue;
 		rec = bsearch(&key, pg->records, pg->index,
-- 
2.39.1

[for-linus][PATCH 4/5] ftrace,kcfi: Define ftrace_stub_graph conditionally

[Steven Rostedt]

From: Arnd Bergmann <arnd@arndb.de>

When CONFIG_FUNCTION_GRAPH_TRACER is disabled, __kcfi_typeid_ftrace_stub_graph
is missing, causing a link failure:

 ld.lld: error: undefined symbol: __kcfi_typeid_ftrace_stub_graph
 referenced by arch/x86/kernel/ftrace_64.o:(__cfi_ftrace_stub_graph) in archive vmlinux.a

Mark the reference to it as conditional on the same symbol, as
is done on arm64.

Link: https://lore.kernel.org/linux-trace-kernel/20230131093643.3850272-1-arnd@kernel.org

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Fixes: 883bbbffa5a4 ("ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()")
See-also: 2598ac6ec493 ("arm64: ftrace: Define ftrace_stub_graph only with FUNCTION_GRAPH_TRACER")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace_64.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 1265ad519249..fb4f1e01b64a 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -136,10 +136,12 @@ SYM_TYPED_FUNC_START(ftrace_stub)
 	RET
 SYM_FUNC_END(ftrace_stub)
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 SYM_TYPED_FUNC_START(ftrace_stub_graph)
 	CALL_DEPTH_ACCOUNT
 	RET
 SYM_FUNC_END(ftrace_stub_graph)
+#endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-- 
2.39.1

[for-linus][PATCH 5/5] tracing: Make tracepoint lockdep check actually test something

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

A while ago where the trace events had the following:

   rcu_read_lock_sched_notrace();
   rcu_dereference_sched(...);
   rcu_read_unlock_sched_notrace();

If the tracepoint is enabled, it could trigger RCU issues if called in
the wrong place. And this warning was only triggered if lockdep was
enabled. If the tracepoint was never enabled with lockdep, the bug would
not be caught. To handle this, the above sequence was done when lockdep
was enabled regardless if the tracepoint was enabled or not (although the
always enabled code really didn't do anything, it would still trigger a
warning).

But a lot has changed since that lockdep code was added. One is, that
sequence no longer triggers any warning. Another is, the tracepoint when
enabled doesn't even do that sequence anymore.

The main check we care about today is whether RCU is "watching" or not.
So if lockdep is enabled, always check if rcu_is_watching() which will
trigger a warning if it is not (tracepoints require RCU to be watching).

Note, that old sequence did add a bit of overhead when lockdep was enabled,
and with the latest kernel updates, would cause the system to slow down
enough to trigger kernel "stalled" warnings.

Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com
Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com
Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20230310172856.77406446@gandalf.local.home

Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/linux/tracepoint.h | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index fa1004fcf810..2083f2d2f05b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -231,12 +231,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  *
- * When lockdep is enabled, we make sure to always do the RCU portions of
- * the tracepoint code, regardless of whether tracing is on. However,
- * don't check if the condition is false, due to interaction with idle
- * instrumentation. This lets us find RCU issues triggered with tracepoints
- * even when this tracepoint is off. This code has no purpose other than
- * poking RCU a bit.
+ * When lockdep is enabled, we make sure to always test if RCU is
+ * "watching" regardless if the tracepoint is enabled or not. Tracepoints
+ * require RCU to be active, and it should always warn at the tracepoint
+ * site if it is not watching, as it will need to be active when the
+ * tracepoint is enabled.
  */
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
 	extern int __traceiter_##name(data_proto);			\
@@ -249,9 +248,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 				TP_ARGS(args),				\
 				TP_CONDITION(cond), 0);			\
 		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
-			rcu_read_lock_sched_notrace();			\
-			rcu_dereference_sched(__tracepoint_##name.funcs);\
-			rcu_read_unlock_sched_notrace();		\
+			WARN_ON_ONCE(!rcu_is_watching());		\
 		}							\
 	}								\
 	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
-- 
2.39.1

Re: [for-linus][PATCH 5/5] tracing: Make tracepoint lockdep check actually test something

[Paul E. McKenney]

On Tue, Mar 14, 2023 at 03:02:41PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> A while ago where the trace events had the following:
> 
>    rcu_read_lock_sched_notrace();
>    rcu_dereference_sched(...);
>    rcu_read_unlock_sched_notrace();
> 
> If the tracepoint is enabled, it could trigger RCU issues if called in
> the wrong place. And this warning was only triggered if lockdep was
> enabled. If the tracepoint was never enabled with lockdep, the bug would
> not be caught. To handle this, the above sequence was done when lockdep
> was enabled regardless if the tracepoint was enabled or not (although the
> always enabled code really didn't do anything, it would still trigger a
> warning).
> 
> But a lot has changed since that lockdep code was added. One is, that
> sequence no longer triggers any warning. Another is, the tracepoint when
> enabled doesn't even do that sequence anymore.
> 
> The main check we care about today is whether RCU is "watching" or not.
> So if lockdep is enabled, always check if rcu_is_watching() which will
> trigger a warning if it is not (tracepoints require RCU to be watching).
> 
> Note, that old sequence did add a bit of overhead when lockdep was enabled,
> and with the latest kernel updates, would cause the system to slow down
> enough to trigger kernel "stalled" warnings.
> 
> Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com
> Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com
> Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/
> Link: https://lore.kernel.org/linux-trace-kernel/20230310172856.77406446@gandalf.local.home
> 
> Cc: stable@vger.kernel.org
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  include/linux/tracepoint.h | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index fa1004fcf810..2083f2d2f05b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -231,12 +231,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   *
> - * When lockdep is enabled, we make sure to always do the RCU portions of
> - * the tracepoint code, regardless of whether tracing is on. However,
> - * don't check if the condition is false, due to interaction with idle
> - * instrumentation. This lets us find RCU issues triggered with tracepoints
> - * even when this tracepoint is off. This code has no purpose other than
> - * poking RCU a bit.
> + * When lockdep is enabled, we make sure to always test if RCU is
> + * "watching" regardless if the tracepoint is enabled or not. Tracepoints
> + * require RCU to be active, and it should always warn at the tracepoint
> + * site if it is not watching, as it will need to be active when the
> + * tracepoint is enabled.
>   */
>  #define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
>  	extern int __traceiter_##name(data_proto);			\
> @@ -249,9 +248,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  				TP_ARGS(args),				\
>  				TP_CONDITION(cond), 0);			\
>  		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
> -			rcu_read_lock_sched_notrace();			\
> -			rcu_dereference_sched(__tracepoint_##name.funcs);\
> -			rcu_read_unlock_sched_notrace();		\
> +			WARN_ON_ONCE(!rcu_is_watching());		\
>  		}							\
>  	}								\
>  	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
> -- 
> 2.39.1

Re: [for-linus][PATCH 5/5] tracing: Make tracepoint lockdep check actually test something

[Steven Rostedt]

On Tue, 14 Mar 2023 14:08:28 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Tue, Mar 14, 2023 at 03:02:41PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > A while ago where the trace events had the following:
> > 
> >    rcu_read_lock_sched_notrace();
> >    rcu_dereference_sched(...);
> >    rcu_read_unlock_sched_notrace();
> > 
> > If the tracepoint is enabled, it could trigger RCU issues if called in
> > the wrong place. And this warning was only triggered if lockdep was
> > enabled. If the tracepoint was never enabled with lockdep, the bug would
> > not be caught. To handle this, the above sequence was done when lockdep
> > was enabled regardless if the tracepoint was enabled or not (although the
> > always enabled code really didn't do anything, it would still trigger a
> > warning).
> > 
> > But a lot has changed since that lockdep code was added. One is, that
> > sequence no longer triggers any warning. Another is, the tracepoint when
> > enabled doesn't even do that sequence anymore.
> > 
> > The main check we care about today is whether RCU is "watching" or not.
> > So if lockdep is enabled, always check if rcu_is_watching() which will
> > trigger a warning if it is not (tracepoints require RCU to be watching).
> > 
> > Note, that old sequence did add a bit of overhead when lockdep was enabled,
> > and with the latest kernel updates, would cause the system to slow down
> > enough to trigger kernel "stalled" warnings.
> > 
> > Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com
> > Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com
> > Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/
> > Link: https://lore.kernel.org/linux-trace-kernel/20230310172856.77406446@gandalf.local.home
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> 
> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> 

Thanks Paul!

-- Steve

Re: [for-linus][PATCH 5/5] tracing: Make tracepoint lockdep check actually test something

[Joel Fernandes]

On Tue, Mar 14, 2023 at 3:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> A while ago where the trace events had the following:
>
>    rcu_read_lock_sched_notrace();
>    rcu_dereference_sched(...);
>    rcu_read_unlock_sched_notrace();
>
> If the tracepoint is enabled, it could trigger RCU issues if called in
> the wrong place. And this warning was only triggered if lockdep was
> enabled. If the tracepoint was never enabled with lockdep, the bug would
> not be caught. To handle this, the above sequence was done when lockdep
> was enabled regardless if the tracepoint was enabled or not (although the
> always enabled code really didn't do anything, it would still trigger a
> warning).
>
> But a lot has changed since that lockdep code was added. One is, that
> sequence no longer triggers any warning. Another is, the tracepoint when
> enabled doesn't even do that sequence anymore.

I agree with the change but I am confused by the commit message a bit
due to "Another is, the tracepoint when enabled doesn't even do that
sequence anymore.".

Whether the tracepoint was enabled or disabled, it is always doing the
old sequence because we were skipping the tracepoint's static key test
before running the sequence. Right?

So how was it not doing the old sequence before?

Other than that,
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

 - Joel


> The main check we care about today is whether RCU is "watching" or not.
> So if lockdep is enabled, always check if rcu_is_watching() which will
> trigger a warning if it is not (tracepoints require RCU to be watching).
>
> Note, that old sequence did add a bit of overhead when lockdep was enabled,
> and with the latest kernel updates, would cause the system to slow down
> enough to trigger kernel "stalled" warnings.
>
> Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com
> Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com
> Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/
> Link: https://lore.kernel.org/linux-trace-kernel/20230310172856.77406446@gandalf.local.home
>
> Cc: stable@vger.kernel.org
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/linux/tracepoint.h | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index fa1004fcf810..2083f2d2f05b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -231,12 +231,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   *
> - * When lockdep is enabled, we make sure to always do the RCU portions of
> - * the tracepoint code, regardless of whether tracing is on. However,
> - * don't check if the condition is false, due to interaction with idle
> - * instrumentation. This lets us find RCU issues triggered with tracepoints
> - * even when this tracepoint is off. This code has no purpose other than
> - * poking RCU a bit.
> + * When lockdep is enabled, we make sure to always test if RCU is
> + * "watching" regardless if the tracepoint is enabled or not. Tracepoints
> + * require RCU to be active, and it should always warn at the tracepoint
> + * site if it is not watching, as it will need to be active when the
> + * tracepoint is enabled.
>   */
>  #define __DECLARE_TRACE(name, proto, args, cond, data_proto)           \
>         extern int __traceiter_##name(data_proto);                      \
> @@ -249,9 +248,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>                                 TP_ARGS(args),                          \
>                                 TP_CONDITION(cond), 0);                 \
>                 if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
> -                       rcu_read_lock_sched_notrace();                  \
> -                       rcu_dereference_sched(__tracepoint_##name.funcs);\
> -                       rcu_read_unlock_sched_notrace();                \
> +                       WARN_ON_ONCE(!rcu_is_watching());               \
>                 }                                                       \
>         }                                                               \
>         __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),          \
> --
> 2.39.1

Re: [for-linus][PATCH 5/5] tracing: Make tracepoint lockdep check actually test something

[Joel Fernandes]

On Tue, Mar 14, 2023 at 7:03 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Tue, Mar 14, 2023 at 3:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> >
> > A while ago where the trace events had the following:
> >
> >    rcu_read_lock_sched_notrace();
> >    rcu_dereference_sched(...);
> >    rcu_read_unlock_sched_notrace();
> >
> > If the tracepoint is enabled, it could trigger RCU issues if called in
> > the wrong place. And this warning was only triggered if lockdep was
> > enabled. If the tracepoint was never enabled with lockdep, the bug would
> > not be caught. To handle this, the above sequence was done when lockdep
> > was enabled regardless if the tracepoint was enabled or not (although the
> > always enabled code really didn't do anything, it would still trigger a
> > warning).
> >
> > But a lot has changed since that lockdep code was added. One is, that
> > sequence no longer triggers any warning. Another is, the tracepoint when
> > enabled doesn't even do that sequence anymore.
>
> I agree with the change but I am confused by the commit message a bit
> due to "Another is, the tracepoint when enabled doesn't even do that
> sequence anymore.".
>
> Whether the tracepoint was enabled or disabled, it is always doing the
> old sequence because we were skipping the tracepoint's static key test
> before running the sequence. Right?
>
> So how was it not doing the old sequence before?

Ah I see, you meant "It was doing a dummy de-ref", not that "it was
_not_ doing anything". ;-)

So it is good then, but perhaps (optionally) call the code as a dummy
RCU deref which was supposed to trigger a warning. ;-)

 - Joel


>
> Other than that,
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
>  - Joel
>
>
> > The main check we care about today is whether RCU is "watching" or not.
> > So if lockdep is enabled, always check if rcu_is_watching() which will
> > trigger a warning if it is not (tracepoints require RCU to be watching).
> >
> > Note, that old sequence did add a bit of overhead when lockdep was enabled,
> > and with the latest kernel updates, would cause the system to slow down
> > enough to trigger kernel "stalled" warnings.
> >
> > Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com
> > Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com
> > Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/
> > Link: https://lore.kernel.org/linux-trace-kernel/20230310172856.77406446@gandalf.local.home
> >
> > Cc: stable@vger.kernel.org
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >  include/linux/tracepoint.h | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index fa1004fcf810..2083f2d2f05b 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -231,12 +231,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >   * not add unwanted padding between the beginning of the section and the
> >   * structure. Force alignment to the same alignment as the section start.
> >   *
> > - * When lockdep is enabled, we make sure to always do the RCU portions of
> > - * the tracepoint code, regardless of whether tracing is on. However,
> > - * don't check if the condition is false, due to interaction with idle
> > - * instrumentation. This lets us find RCU issues triggered with tracepoints
> > - * even when this tracepoint is off. This code has no purpose other than
> > - * poking RCU a bit.
> > + * When lockdep is enabled, we make sure to always test if RCU is
> > + * "watching" regardless if the tracepoint is enabled or not. Tracepoints
> > + * require RCU to be active, and it should always warn at the tracepoint
> > + * site if it is not watching, as it will need to be active when the
> > + * tracepoint is enabled.
> >   */
> >  #define __DECLARE_TRACE(name, proto, args, cond, data_proto)           \
> >         extern int __traceiter_##name(data_proto);                      \
> > @@ -249,9 +248,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >                                 TP_ARGS(args),                          \
> >                                 TP_CONDITION(cond), 0);                 \
> >                 if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
> > -                       rcu_read_lock_sched_notrace();                  \
> > -                       rcu_dereference_sched(__tracepoint_##name.funcs);\
> > -                       rcu_read_unlock_sched_notrace();                \
> > +                       WARN_ON_ONCE(!rcu_is_watching());               \
> >                 }                                                       \
> >         }                                                               \
> >         __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),          \
> > --
> > 2.39.1