[for-linus][PATCH 00/10] tracing: Fixes for 6.0

[for-linus][PATCH 00/10] tracing: Fixes for 6.0

[Steven Rostedt]

Various fixes for tracing:

 - Suppress the errors that is_signed_type() causes static analysis tools

 - Fix a return value of traceprobe_parse_event_name()

 - Fix NULL pointer dereference from failed ftrace enabling

 - Fix NULL pointer dereference by asking for registers from eprobes

 - Make eprobes consistent with kprobes/uprobes, filters and histograms

Bart Van Assche (1):
      tracing: Suppress sparse warnings triggered by is_signed_type()

Lukas Bulwahn (1):
      tracing: React to error return from traceprobe_parse_event_name()

Steven Rostedt (Google) (7):
      tracing/perf: Fix double put of trace event when init fails
      tracing/eprobes: Do not allow eprobes to use $stack, or % for regs
      tracing/eprobes: Do not hardcode $comm as a string
      tracing/eprobes: Fix reading of string fields
      tracing/eprobes: Have event probes be consistent with kprobes and uprobes
      tracing/probes: Have kprobes and uprobes use $COMM too
      tracing: Have filter accept "common_cpu" to be consistent

Yang Jihong (1):
      ftrace: Fix NULL pointer dereference in is_ftrace_trampoline when ftrace is dead

----
 include/linux/trace_events.h    | 12 ++++++
 kernel/trace/ftrace.c           | 10 +++++
 kernel/trace/trace_eprobe.c     | 93 +++++++++++++++++++++++++++++++++++++----
 kernel/trace/trace_event_perf.c |  7 ++--
 kernel/trace/trace_events.c     |  1 +
 kernel/trace/trace_probe.c      | 29 ++++++++-----
 6 files changed, 131 insertions(+), 21 deletions(-)

[for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Steven Rostedt]

From: Bart Van Assche <bvanassche@acm.org>

Using a __bitwise type in a tracing __field() definition triggers four
sparse warnings in stage 4 of expanding the TRACE_EVENT() macro. These
warnings are triggered by the is_signed_type() macro implementation.
Since there is no known way of checking signedness of a bitwise type
without triggering sparse warnings, disable signedness checking when
verifying code with sparse.

Note: work is in progress to improve sparse but has not yet landed. See
also "[PATCH 0/5] allow -1 and compares in bitwise types"
(https://lore.kernel.org/all/20220627190540.13358-1-luc.vanoostenryck@gmail.com/ ).

Link: https://lore.kernel.org/linux-trace-devel/20220717151047.19220-1-bvanassche@acm.org

Suggested-by: Christoph Hellwig <hch@lst.de>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/linux/trace_events.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..b60347a0ccde 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -814,7 +814,19 @@ extern int trace_add_event_call(struct trace_event_call *call);
 extern int trace_remove_event_call(struct trace_event_call *call);
 extern int trace_event_get_offsets(struct trace_event_call *call);
 
+/*
+ * There is no known way to check signedness of __bitwise types without
+ * triggering a sparse warning. Hence the #ifdef __CHECKER__.
+ *
+ * Since there is another definition of is_signed_type() in <linux/overflow.h>,
+ * undefine is_signed_type() before redefining it.
+ */
+#undef is_signed_type
+#ifdef __CHECKER__
+#define is_signed_type(type)	0
+#else
 #define is_signed_type(type)	(((type)(-1)) < (type)1)
+#endif
 
 int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
 int trace_set_clr_event(const char *system, const char *event, int set);
-- 
2.35.1

[for-linus][PATCH 02/10] tracing: React to error return from traceprobe_parse_event_name()

[Steven Rostedt]

From: Lukas Bulwahn <lukas.bulwahn@gmail.com>

The function traceprobe_parse_event_name() may set the first two function
arguments to a non-null value and still return -EINVAL to indicate an
unsuccessful completion of the function. Hence, it is not sufficient to
just check the result of the two function arguments for being not null,
but the return value also needs to be checked.

Commit 95c104c378dc ("tracing: Auto generate event name when creating a
group of events") changed the error-return-value checking of the second
traceprobe_parse_event_name() invocation in __trace_eprobe_create() and
removed checking the return value to jump to the error handling case.

Reinstate using the return value in the error-return-value checking.

Link: https://lkml.kernel.org/r/20220811071734.20700-1-lukas.bulwahn@gmail.com

Fixes: 95c104c378dc ("tracing: Auto generate event name when creating a group of events")
Acked-by: Linyu Yuan <quic_linyyuan@quicinc.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_eprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 4a0e9d927443..550671985fd1 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -883,7 +883,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	trace_probe_log_set_index(1);
 	sys_event = argv[1];
 	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
-	if (!sys_event || !sys_name) {
+	if (ret || !sys_event || !sys_name) {
 		trace_probe_log_err(0, NO_EVENT_INFO);
 		goto parse_error;
 	}
-- 
2.35.1

[for-linus][PATCH 03/10] tracing/perf: Fix double put of trace event when init fails

[Steven Rostedt]

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

If in perf_trace_event_init(), the perf_trace_event_open() fails, then it
will call perf_trace_event_unreg() which will not only unregister the perf
trace event, but will also call the put() function of the tp_event.

The problem here is that the trace_event_try_get_ref() is called by the
caller of perf_trace_event_init() and if perf_trace_event_init() returns a
failure, it will then call trace_event_put(). But since the
perf_trace_event_unreg() already called the trace_event_put() function, it
triggers a WARN_ON().

 WARNING: CPU: 1 PID: 30309 at kernel/trace/trace_dynevent.c:46 trace_event_dyn_put_ref+0x15/0x20

If perf_trace_event_reg() does not call the trace_event_try_get_ref() then
the perf_trace_event_unreg() should not be calling trace_event_put(). This
breaks symmetry and causes bugs like these.

Pull out the trace_event_put() from perf_trace_event_unreg() and call it
in the locations that perf_trace_event_unreg() is called. This not only
fixes this bug, but also brings back the proper symmetry of the reg/unreg
vs get/put logic.

Link: https://lore.kernel.org/all/cover.1660347763.git.kjlx@templeofstupid.com/
Link: https://lkml.kernel.org/r/20220816192817.43d5e17f@gandalf.local.home

Cc: stable@vger.kernel.org
Fixes: 1d18538e6a092 ("tracing: Have dynamic events have a ref counter")
Reported-by: Krister Johansen <kjlx@templeofstupid.com>
Reviewed-by: Krister Johansen <kjlx@templeofstupid.com>
Tested-by: Krister Johansen <kjlx@templeofstupid.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_event_perf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index a114549720d6..61e3a2620fa3 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -157,7 +157,7 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
 	int i;
 
 	if (--tp_event->perf_refcount > 0)
-		goto out;
+		return;
 
 	tp_event->class->reg(tp_event, TRACE_REG_PERF_UNREGISTER, NULL);
 
@@ -176,8 +176,6 @@ static void perf_trace_event_unreg(struct perf_event *p_event)
 			perf_trace_buf[i] = NULL;
 		}
 	}
-out:
-	trace_event_put_ref(tp_event);
 }
 
 static int perf_trace_event_open(struct perf_event *p_event)
@@ -241,6 +239,7 @@ void perf_trace_destroy(struct perf_event *p_event)
 	mutex_lock(&event_mutex);
 	perf_trace_event_close(p_event);
 	perf_trace_event_unreg(p_event);
+	trace_event_put_ref(p_event->tp_event);
 	mutex_unlock(&event_mutex);
 }
 
@@ -292,6 +291,7 @@ void perf_kprobe_destroy(struct perf_event *p_event)
 	mutex_lock(&event_mutex);
 	perf_trace_event_close(p_event);
 	perf_trace_event_unreg(p_event);
+	trace_event_put_ref(p_event->tp_event);
 	mutex_unlock(&event_mutex);
 
 	destroy_local_trace_kprobe(p_event->tp_event);
@@ -347,6 +347,7 @@ void perf_uprobe_destroy(struct perf_event *p_event)
 	mutex_lock(&event_mutex);
 	perf_trace_event_close(p_event);
 	perf_trace_event_unreg(p_event);
+	trace_event_put_ref(p_event->tp_event);
 	mutex_unlock(&event_mutex);
 	destroy_local_trace_uprobe(p_event->tp_event);
 }
-- 
2.35.1

[for-linus][PATCH 04/10] ftrace: Fix NULL pointer dereference in is_ftrace_trampoline when ftrace is dead

[Steven Rostedt]

From: Yang Jihong <yangjihong1@huawei.com>

ftrace_startup does not remove ops from ftrace_ops_list when
ftrace_startup_enable fails:

register_ftrace_function
  ftrace_startup
    __register_ftrace_function
      ...
      add_ftrace_ops(&ftrace_ops_list, ops)
      ...
    ...
    ftrace_startup_enable // if ftrace failed to modify, ftrace_disabled is set to 1
    ...
  return 0 // ops is in the ftrace_ops_list.

When ftrace_disabled = 1, unregister_ftrace_function simply returns without doing anything:
unregister_ftrace_function
  ftrace_shutdown
    if (unlikely(ftrace_disabled))
            return -ENODEV;  // return here, __unregister_ftrace_function is not executed,
                             // as a result, ops is still in the ftrace_ops_list
    __unregister_ftrace_function
    ...

If ops is dynamically allocated, it will be free later, in this case,
is_ftrace_trampoline accesses NULL pointer:

is_ftrace_trampoline
  ftrace_ops_trampoline
    do_for_each_ftrace_op(op, ftrace_ops_list) // OOPS! op may be NULL!

Syzkaller reports as follows:
[ 1203.506103] BUG: kernel NULL pointer dereference, address: 000000000000010b
[ 1203.508039] #PF: supervisor read access in kernel mode
[ 1203.508798] #PF: error_code(0x0000) - not-present page
[ 1203.509558] PGD 800000011660b067 P4D 800000011660b067 PUD 130fb8067 PMD 0
[ 1203.510560] Oops: 0000 [#1] SMP KASAN PTI
[ 1203.511189] CPU: 6 PID: 29532 Comm: syz-executor.2 Tainted: G    B   W         5.10.0 #8
[ 1203.512324] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[ 1203.513895] RIP: 0010:is_ftrace_trampoline+0x26/0xb0
[ 1203.514644] Code: ff eb d3 90 41 55 41 54 49 89 fc 55 53 e8 f2 00 fd ff 48 8b 1d 3b 35 5d 03 e8 e6 00 fd ff 48 8d bb 90 00 00 00 e8 2a 81 26 00 <48> 8b ab 90 00 00 00 48 85 ed 74 1d e8 c9 00 fd ff 48 8d bb 98 00
[ 1203.518838] RSP: 0018:ffffc900012cf960 EFLAGS: 00010246
[ 1203.520092] RAX: 0000000000000000 RBX: 000000000000007b RCX: ffffffff8a331866
[ 1203.521469] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000000010b
[ 1203.522583] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8df18b07
[ 1203.523550] R10: fffffbfff1be3160 R11: 0000000000000001 R12: 0000000000478399
[ 1203.524596] R13: 0000000000000000 R14: ffff888145088000 R15: 0000000000000008
[ 1203.525634] FS:  00007f429f5f4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
[ 1203.526801] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1203.527626] CR2: 000000000000010b CR3: 0000000170e1e001 CR4: 00000000003706e0
[ 1203.528611] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1203.529605] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

Therefore, when ftrace_startup_enable fails, we need to rollback registration
process and remove ops from ftrace_ops_list.

Link: https://lkml.kernel.org/r/20220818032659.56209-1-yangjihong1@huawei.com

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 601ccf1b2f09..4baa99363b16 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2937,6 +2937,16 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
 
 	ftrace_startup_enable(command);
 
+	/*
+	 * If ftrace is in an undefined state, we just remove ops from list
+	 * to prevent the NULL pointer, instead of totally rolling it back and
+	 * free trampoline, because those actions could cause further damage.
+	 */
+	if (unlikely(ftrace_disabled)) {
+		__unregister_ftrace_function(ops);
+		return -ENODEV;
+	}
+
 	ops->flags &= ~FTRACE_OPS_FL_ADDING;
 
 	return 0;
-- 
2.35.1

[for-linus][PATCH 05/10] tracing/eprobes: Do not allow eprobes to use $stack, or % for regs

[Steven Rostedt]

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

While playing with event probes (eprobes), I tried to see what would
happen if I attempted to retrieve the instruction pointer (%rip) knowing
that event probes do not use pt_regs. The result was:

 BUG: kernel NULL pointer dereference, address: 0000000000000024
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: 0000 [#1] PREEMPT SMP PTI
 CPU: 1 PID: 1847 Comm: trace-cmd Not tainted 5.19.0-rc5-test+ #309
 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01
v03.03 07/14/2016
 RIP: 0010:get_event_field.isra.0+0x0/0x50
 Code: ff 48 c7 c7 c0 8f 74 a1 e8 3d 8b f5 ff e8 88 09 f6 ff 4c 89 e7 e8
50 6a 13 00 48 89 ef 5b 5d 41 5c 41 5d e9 42 6a 13 00 66 90 <48> 63 47 24
8b 57 2c 48 01 c6 8b 47 28 83 f8 02 74 0e 83 f8 04 74
 RSP: 0018:ffff916c394bbaf0 EFLAGS: 00010086
 RAX: ffff916c854041d8 RBX: ffff916c8d9fbf50 RCX: ffff916c255d2000
 RDX: 0000000000000000 RSI: ffff916c255d2008 RDI: 0000000000000000
 RBP: 0000000000000000 R08: ffff916c3a2a0c08 R09: ffff916c394bbda8
 R10: 0000000000000000 R11: 0000000000000000 R12: ffff916c854041d8
 R13: ffff916c854041b0 R14: 0000000000000000 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff916c9ea40000(0000)
knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000024 CR3: 000000011b60a002 CR4: 00000000001706e0
 Call Trace:
  <TASK>
  get_eprobe_size+0xb4/0x640
  ? __mod_node_page_state+0x72/0xc0
  __eprobe_trace_func+0x59/0x1a0
  ? __mod_lruvec_page_state+0xaa/0x1b0
  ? page_remove_file_rmap+0x14/0x230
  ? page_remove_rmap+0xda/0x170
  event_triggers_call+0x52/0xe0
  trace_event_buffer_commit+0x18f/0x240
  trace_event_raw_event_sched_wakeup_template+0x7a/0xb0
  try_to_wake_up+0x260/0x4c0
  __wake_up_common+0x80/0x180
  __wake_up_common_lock+0x7c/0xc0
  do_notify_parent+0x1c9/0x2a0
  exit_notify+0x1a9/0x220
  do_exit+0x2ba/0x450
  do_group_exit+0x2d/0x90
  __x64_sys_exit_group+0x14/0x20
  do_syscall_64+0x3b/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0

Obviously this is not the desired result.

Move the testing for TPARG_FL_TPOINT which is only used for event probes
to the top of the "$" variable check, as all the other variables are not
used for event probes. Also add a check in the register parsing "%" to
fail if an event probe is used.

Cc: stable@vger.kernel.org
Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 850a88abd33b..dec657af363c 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -283,7 +283,14 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
 	int ret = 0;
 	int len;
 
-	if (strcmp(arg, "retval") == 0) {
+	if (flags & TPARG_FL_TPOINT) {
+		if (code->data)
+			return -EFAULT;
+		code->data = kstrdup(arg, GFP_KERNEL);
+		if (!code->data)
+			return -ENOMEM;
+		code->op = FETCH_OP_TP_ARG;
+	} else if (strcmp(arg, "retval") == 0) {
 		if (flags & TPARG_FL_RETURN) {
 			code->op = FETCH_OP_RETVAL;
 		} else {
@@ -323,13 +330,6 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
 		code->op = FETCH_OP_ARG;
 		code->param = (unsigned int)param - 1;
 #endif
-	} else if (flags & TPARG_FL_TPOINT) {
-		if (code->data)
-			return -EFAULT;
-		code->data = kstrdup(arg, GFP_KERNEL);
-		if (!code->data)
-			return -ENOMEM;
-		code->op = FETCH_OP_TP_ARG;
 	} else
 		goto inval_var;
 
@@ -384,6 +384,11 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 		break;
 
 	case '%':	/* named register */
+		if (flags & TPARG_FL_TPOINT) {
+			/* eprobes do not handle registers */
+			trace_probe_log_err(offs, BAD_VAR);
+			break;
+		}
 		ret = regs_query_register_offset(arg + 1);
 		if (ret >= 0) {
 			code->op = FETCH_OP_REG;
-- 
2.35.1

[for-linus][PATCH 06/10] tracing/eprobes: Do not hardcode $comm as a string

[Steven Rostedt]

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

The variable $comm is hard coded as a string, which is true for both
kprobes and uprobes, but for event probes (eprobes) it is a field name. In
most cases the "comm" field would be a string, but there's no guarantee of
that fact.

Do not assume that comm is a string. Not to mention, it currently forces
comm fields to fault, as string processing for event probes is currently
broken.

Cc: stable@vger.kernel.org
Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index dec657af363c..4daabbb8b772 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -622,9 +622,10 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 
 	/*
 	 * Since $comm and immediate string can not be dereferenced,
-	 * we can find those by strcmp.
+	 * we can find those by strcmp. But ignore for eprobes.
 	 */
-	if (strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0) {
+	if (!(flags & TPARG_FL_TPOINT) &&
+	    (strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0)) {
 		/* The type of $comm must be "string", and not an array. */
 		if (parg->count || (t && strcmp(t, "string")))
 			goto out;
-- 
2.35.1

[for-linus][PATCH 07/10] tracing/eprobes: Fix reading of string fields

[Steven Rostedt]

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

Currently when an event probe (eprobe) hooks to a string field, it does
not display it as a string, but instead as a number. This makes the field
rather useless. Handle the different kinds of strings, dynamic, static,
relational/dynamic etc.

Now when a string field is used, the ":string" type can be used to display
it:

  echo "e:sw sched/sched_switch comm=$next_comm:string" > dynamic_events

Cc: stable@vger.kernel.org
Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_eprobe.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 550671985fd1..a1d3423ab74f 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -311,6 +311,27 @@ static unsigned long get_event_field(struct fetch_insn *code, void *rec)
 
 	addr = rec + field->offset;
 
+	if (is_string_field(field)) {
+		switch (field->filter_type) {
+		case FILTER_DYN_STRING:
+			val = (unsigned long)(rec + (*(unsigned int *)addr & 0xffff));
+			break;
+		case FILTER_RDYN_STRING:
+			val = (unsigned long)(addr + (*(unsigned int *)addr & 0xffff));
+			break;
+		case FILTER_STATIC_STRING:
+			val = (unsigned long)addr;
+			break;
+		case FILTER_PTR_STRING:
+			val = (unsigned long)(*(char *)addr);
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			return 0;
+		}
+		return val;
+	}
+
 	switch (field->size) {
 	case 1:
 		if (field->is_signed)
-- 
2.35.1

[for-linus][PATCH 08/10] tracing/eprobes: Have event probes be consistent with kprobes and uprobes

[Steven Rostedt]

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

Currently, if a symbol "@" is attempted to be used with an event probe
(eprobes), it will cause a NULL pointer dereference crash.

Both kprobes and uprobes can reference data other than the main registers.
Such as immediate address, symbols and the current task name. Have eprobes
do the same thing.

For "comm", if "comm" is used and the event being attached to does not
have the "comm" field, then make it the "$comm" that kprobes has. This is
consistent to the way histograms and filters work.

Cc: stable@vger.kernel.org
Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_eprobe.c | 70 +++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index a1d3423ab74f..1783e3478912 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -227,6 +227,7 @@ static int trace_eprobe_tp_arg_update(struct trace_eprobe *ep, int i)
 	struct probe_arg *parg = &ep->tp.args[i];
 	struct ftrace_event_field *field;
 	struct list_head *head;
+	int ret = -ENOENT;
 
 	head = trace_get_fields(ep->event);
 	list_for_each_entry(field, head, link) {
@@ -236,9 +237,20 @@ static int trace_eprobe_tp_arg_update(struct trace_eprobe *ep, int i)
 			return 0;
 		}
 	}
+
+	/*
+	 * Argument not found on event. But allow for comm and COMM
+	 * to be used to get the current->comm.
+	 */
+	if (strcmp(parg->code->data, "COMM") == 0 ||
+	    strcmp(parg->code->data, "comm") == 0) {
+		parg->code->op = FETCH_OP_COMM;
+		ret = 0;
+	}
+
 	kfree(parg->code->data);
 	parg->code->data = NULL;
-	return -ENOENT;
+	return ret;
 }
 
 static int eprobe_event_define_fields(struct trace_event_call *event_call)
@@ -363,16 +375,38 @@ static unsigned long get_event_field(struct fetch_insn *code, void *rec)
 
 static int get_eprobe_size(struct trace_probe *tp, void *rec)
 {
+	struct fetch_insn *code;
 	struct probe_arg *arg;
 	int i, len, ret = 0;
 
 	for (i = 0; i < tp->nr_args; i++) {
 		arg = tp->args + i;
-		if (unlikely(arg->dynamic)) {
+		if (arg->dynamic) {
 			unsigned long val;
 
-			val = get_event_field(arg->code, rec);
-			len = process_fetch_insn_bottom(arg->code + 1, val, NULL, NULL);
+			code = arg->code;
+ retry:
+			switch (code->op) {
+			case FETCH_OP_TP_ARG:
+				val = get_event_field(code, rec);
+				break;
+			case FETCH_OP_IMM:
+				val = code->immediate;
+				break;
+			case FETCH_OP_COMM:
+				val = (unsigned long)current->comm;
+				break;
+			case FETCH_OP_DATA:
+				val = (unsigned long)code->data;
+				break;
+			case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
+				code++;
+				goto retry;
+			default:
+				continue;
+			}
+			code++;
+			len = process_fetch_insn_bottom(code, val, NULL, NULL);
 			if (len > 0)
 				ret += len;
 		}
@@ -390,8 +424,28 @@ process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
 {
 	unsigned long val;
 
-	val = get_event_field(code, rec);
-	return process_fetch_insn_bottom(code + 1, val, dest, base);
+ retry:
+	switch (code->op) {
+	case FETCH_OP_TP_ARG:
+		val = get_event_field(code, rec);
+		break;
+	case FETCH_OP_IMM:
+		val = code->immediate;
+		break;
+	case FETCH_OP_COMM:
+		val = (unsigned long)current->comm;
+		break;
+	case FETCH_OP_DATA:
+		val = (unsigned long)code->data;
+		break;
+	case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
+		code++;
+		goto retry;
+	default:
+		return -EILSEQ;
+	}
+	code++;
+	return process_fetch_insn_bottom(code, val, dest, base);
 }
 NOKPROBE_SYMBOL(process_fetch_insn)
 
@@ -866,6 +920,10 @@ static int trace_eprobe_tp_update_arg(struct trace_eprobe *ep, const char *argv[
 			trace_probe_log_err(0, BAD_ATTACH_ARG);
 	}
 
+	/* Handle symbols "@" */
+	if (!ret)
+		ret = traceprobe_update_arg(&ep->tp.args[i]);
+
 	return ret;
 }
 
-- 
2.35.1

[for-linus][PATCH 09/10] tracing/probes: Have kprobes and uprobes use $COMM too

[Steven Rostedt]

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

Both $comm and $COMM can be used to get current->comm in eprobes and the
filtering and histogram logic. Make kprobes and uprobes consistent in this
regard and allow both $comm and $COMM as well. Currently kprobes and
uprobes only handle $comm, which is inconsistent with the other utilities,
and can be confusing to users.

Link: https://lore.kernel.org/all/20220820220442.776e1ddaf8836e82edb34d01@kernel.org/

Cc: stable@vger.kernel.org
Fixes: 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 4daabbb8b772..36dff277de46 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -314,7 +314,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
 			}
 		} else
 			goto inval_var;
-	} else if (strcmp(arg, "comm") == 0) {
+	} else if (strcmp(arg, "comm") == 0 || strcmp(arg, "COMM") == 0) {
 		code->op = FETCH_OP_COMM;
 #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
 	} else if (((flags & TPARG_FL_MASK) ==
@@ -625,7 +625,8 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 	 * we can find those by strcmp. But ignore for eprobes.
 	 */
 	if (!(flags & TPARG_FL_TPOINT) &&
-	    (strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0)) {
+	    (strcmp(arg, "$comm") == 0 || strcmp(arg, "$COMM") == 0 ||
+	     strncmp(arg, "\\\"", 2) == 0)) {
 		/* The type of $comm must be "string", and not an array. */
 		if (parg->count || (t && strcmp(t, "string")))
 			goto out;
-- 
2.35.1

[for-linus][PATCH 10/10] tracing: Have filter accept "common_cpu" to be consistent

[Steven Rostedt]

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

Make filtering consistent with histograms. As "cpu" can be a field of an
event, allow for "common_cpu" to keep it from being confused with the
"cpu" field of the event.

Link: https://lore.kernel.org/all/20220820220920.e42fa32b70505b1904f0a0ad@kernel.org/

Cc: stable@vger.kernel.org
Fixes: 1e3bac71c5053 ("tracing/histogram: Rename "cpu" to "common_cpu"")
Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_events.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 181f08186d32..0356cae0cf74 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -176,6 +176,7 @@ static int trace_define_generic_fields(void)
 
 	__generic_field(int, CPU, FILTER_CPU);
 	__generic_field(int, cpu, FILTER_CPU);
+	__generic_field(int, common_cpu, FILTER_CPU);
 	__generic_field(char *, COMM, FILTER_COMM);
 	__generic_field(char *, comm, FILTER_COMM);
 
-- 
2.35.1

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Since there is no known way of checking signedness of a bitwise type
> without triggering sparse warnings, disable signedness checking when
> verifying code with sparse.

What, what, what?

The last I saw of this discussion, the fix was just to make sparse not
warn about these cases. Why did this bogus fix make it into a pull
request that I will now ignore?

If we want to just shut up the sparse warning, then afaik the simple
one-liner fix would have been

-#define is_signed_type(type)   (((type)(-1)) < (type)1)
+#define is_signed_type(type)   (((__force type)(-1)) < (__force type)1)

and at least then sparse checks the same source as is compiled,
instead of passing a "this is not a signed type" to places.

So that "no known way" was always bogus, the real question was whether
there was a way to make sparse not need the "ignore bitwise" hack.

Btw, that patch is entirely broken for *another* reason.

Even if you were to say "ok, sparse just gets a different argument",
the fact that the trace_events file re-defined that is_signed_type()
macro means that you added that

+#undef is_signed_type

to make the compiler happy about how you only modified one of them.

But that then means that if <linux/trace_events.h> gets included
*before* <linux/overflow.h>, you'll just get the warning *there*
instead.

Now, that warning would only happen for a __CHECKER__ build - but
that's the only build this patch is relevant for anyway.

And maybe that ordering doesn't exist, or maybe it only exists on some
very random config. Regardless, it's broken.

Of course, the real fix should be to just not re-define that macro at
all, and just have it in *one* place.

                  Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Steven Rostedt]

On Sun, 21 Aug 2022 11:35:29 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Since there is no known way of checking signedness of a bitwise type
> > without triggering sparse warnings, disable signedness checking when
> > verifying code with sparse.  
> 
> What, what, what?
> 
> The last I saw of this discussion, the fix was just to make sparse not
> warn about these cases. Why did this bogus fix make it into a pull
> request that I will now ignore?

Sorry, I was triaging my internal patchwork and saw the "Suggested-by"
Christoph and was thinking this was what we decided on.

> 
> If we want to just shut up the sparse warning, then afaik the simple
> one-liner fix would have been
> 
> -#define is_signed_type(type)   (((type)(-1)) < (type)1)
> +#define is_signed_type(type)   (((__force type)(-1)) < (__force type)1)
> 
> and at least then sparse checks the same source as is compiled,
> instead of passing a "this is not a signed type" to places.
> 
> So that "no known way" was always bogus, the real question was whether
> there was a way to make sparse not need the "ignore bitwise" hack.
> 
> Btw, that patch is entirely broken for *another* reason.
> 
> Even if you were to say "ok, sparse just gets a different argument",
> the fact that the trace_events file re-defined that is_signed_type()
> macro means that you added that
> 
> +#undef is_signed_type
> 
> to make the compiler happy about how you only modified one of them.
> 
> But that then means that if <linux/trace_events.h> gets included
> *before* <linux/overflow.h>, you'll just get the warning *there*
> instead.
> 
> Now, that warning would only happen for a __CHECKER__ build - but
> that's the only build this patch is relevant for anyway.
> 
> And maybe that ordering doesn't exist, or maybe it only exists on some
> very random config. Regardless, it's broken.
> 
> Of course, the real fix should be to just not re-define that macro at
> all, and just have it in *one* place.


I'll remove this patch and send another pull request.

Thanks,

-- Steve

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/21/22 11:35, Linus Torvalds wrote:
> If we want to just shut up the sparse warning, then afaik the simple
> one-liner fix would have been
> 
> -#define is_signed_type(type)   (((type)(-1)) < (type)1)
> +#define is_signed_type(type)   (((__force type)(-1)) < (__force type)1)
> 
> and at least then sparse checks the same source as is compiled,
> instead of passing a "this is not a signed type" to places.

Hi Linus,

I agree that it's better that sparse sees the same code as what is used to
build the kernel. However, I do not agree that the patch above is a solution.
Sparse reports a warning for the suggested definition above of is_signed_type()
because the new definition tries to use the less-than (<) operator to compare
two __bitwise types.

> Btw, that patch is entirely broken for *another* reason.
> 
> Even if you were to say "ok, sparse just gets a different argument",
> the fact that the trace_events file re-defined that is_signed_type()
> macro means that you added that
> 
> +#undef is_signed_type
> 
> to make the compiler happy about how you only modified one of them.
> 
> But that then means that if <linux/trace_events.h> gets included
> *before* <linux/overflow.h>, you'll just get the warning *there*
> instead.
> 
> Now, that warning would only happen for a __CHECKER__ build - but
> that's the only build this patch is relevant for anyway.
> 
> And maybe that ordering doesn't exist, or maybe it only exists on some
> very random config. Regardless, it's broken.
> 
> Of course, the real fix should be to just not re-define that macro at
> all, and just have it in *one* place.

Agreed that is_signed_type() should only be defined once. If nobody else
beats me to this I will prepare a patch to fix this.

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> I agree that it's better that sparse sees the same code as what is used to
> build the kernel. However, I do not agree that the patch above is a solution.
> Sparse reports a warning for the suggested definition above of is_signed_type()
> because the new definition tries to use the less-than (<) operator to compare
> two __bitwise types.

Argh. I forgot that part. It wasn't just the cast that warned, it was
the compare too.

But we did have a sparse fix for it, didn't we? That fix required that
the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
sparse, but it at least avoided the problem.

          Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/22/22 11:45, Linus Torvalds wrote:
> On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> I agree that it's better that sparse sees the same code as what is used to
>> build the kernel. However, I do not agree that the patch above is a solution.
>> Sparse reports a warning for the suggested definition above of is_signed_type()
>> because the new definition tries to use the less-than (<) operator to compare
>> two __bitwise types.
> 
> Argh. I forgot that part. It wasn't just the cast that warned, it was
> the compare too.
> 
> But we did have a sparse fix for it, didn't we? That fix required that
> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
> sparse, but it at least avoided the problem.

Hi Linus,

I haven't seen any progress on the plan to modify sparse. Hence my attempt to
modify the is_signed_type() definition in the kernel.

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Rasmus Villemoes]

On 22/08/2022 20.45, Linus Torvalds wrote:
> On Mon, Aug 22, 2022 at 11:20 AM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> I agree that it's better that sparse sees the same code as what is used to
>> build the kernel. However, I do not agree that the patch above is a solution.
>> Sparse reports a warning for the suggested definition above of is_signed_type()
>> because the new definition tries to use the less-than (<) operator to compare
>> two __bitwise types.
> 
> Argh. I forgot that part. It wasn't just the cast that warned, it was
> the compare too.
> 
> But we did have a sparse fix for it, didn't we? That fix required that
> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
> sparse, but it at least avoided the problem.

Heh. I originally wrote the comparison "< (t)1" instead of "< (t)0" to
avoid a -Wtype-limits warning when applied to unsigned types - yeah
yeah, the kernel isn't built with that, but it's a nice macro to
copy-paste to other projects, and sometimes people do explicitly enable
-Wtype-limits to manually go through some, and then it's nice to not
have tons of false positives from this macro.

But of course <1 is the same as <=0, and we can indeed spell it that way
without triggering Wtype-limits. So if that can help with also silencing
sparse, ack from me on that part.

Rasmus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/23/22 00:06, Rasmus Villemoes wrote:
> On 22/08/2022 20.45, Linus Torvalds wrote:
>> But we did have a sparse fix for it, didn't we? That fix required that
>> the '< (type)1' cast be changed to '<= (type)0' iirc, and a patch to
>> sparse, but it at least avoided the problem.
> 
> Heh. I originally wrote the comparison "< (t)1" instead of "< (t)0" to
> avoid a -Wtype-limits warning when applied to unsigned types - yeah
> yeah, the kernel isn't built with that, but it's a nice macro to
> copy-paste to other projects, and sometimes people do explicitly enable
> -Wtype-limits to manually go through some, and then it's nice to not
> have tons of false positives from this macro.
> 
> But of course <1 is the same as <=0, and we can indeed spell it that way
> without triggering Wtype-limits. So if that can help with also silencing
> sparse, ack from me on that part.

Thank you Rasmus for having shared this information. Since sparse will 
have to be modified anyway, how about extending it such that the bitwise 
attribute can be removed from a type, e.g. via a new no_bitwise 
attribute? Wouldn't that be a more generic solution than only 
suppressing sparse complaints when comparing compile-time constants and 
when the left-hand-side and right-hand-side have different bitwise 
attributes? For reference purposes, this is how __bitwise is defined:

#ifdef __CHECKER__
#define __bitwise	__attribute__((bitwise))
#else
#define __bitwise
#endif

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 3:05 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Thank you Rasmus for having shared this information. Since sparse will
> have to be modified anyway, how about extending it such that the bitwise
> attribute can be removed from a type, e.g. via a new no_bitwise
> attribute?

I think it's actually easier to just make sparse happy.

Can you try the sparse version at

   git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git

which I just set up temporarily with some patches of mine. It also
makes that '__cond_acquires' thing work that refcount_dec_and_lock()
uses.

It does require that kernel change to make

  #define is_signed_type(type)   (((type)(-1)) <= (type)0)

in both places, since only "no bits set" and "all bits set" are
special values for bitwise types.

Those patches of mine are fairly hacky, and I think Luc would probably
do it differently, but apart from the very last one, they aren't
actively disgusting.

                Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Can you try the sparse version at
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
>
> which I just set up temporarily with some patches of mine.

Ugh, and while testing this with sparse, I noticed that sparse itself
got that whole 'is_signed_type()' check wrong.

The sparse fix was to remove one line of code, but that one worries
me, because that one line was clearly very intentional:

   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d

Adding Al, since he's actually the original source of that bitwise
code (and did a lot of other sparse code on the type handling and
preprocessor side in particular).

             Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/23/22 16:18, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 3:05 PM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> Thank you Rasmus for having shared this information. Since sparse will
>> have to be modified anyway, how about extending it such that the bitwise
>> attribute can be removed from a type, e.g. via a new no_bitwise
>> attribute?
> 
> I think it's actually easier to just make sparse happy.
> 
> Can you try the sparse version at
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> 
> which I just set up temporarily with some patches of mine. It also
> makes that '__cond_acquires' thing work that refcount_dec_and_lock()
> uses.
> 
> It does require that kernel change to make
> 
>    #define is_signed_type(type)   (((type)(-1)) <= (type)0)
> 
> in both places, since only "no bits set" and "all bits set" are
> special values for bitwise types.
> 
> Those patches of mine are fairly hacky, and I think Luc would probably
> do it differently, but apart from the very last one, they aren't
> actively disgusting.

Hi Linus,

I'm probably doing something wrong but even with sparse commit 658ee8e0f631
("unrestricted values are unrestricted even after a cast") I see warnings
being triggered by users of the is_signed_type() macro, warnings that
disappear if I change the definition of the is_signed_type() macro into 0:

$ make C=2 fs/f2fs/ </dev/null |& grep blk_opf_t
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1027:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer
./include/trace/events/f2fs.h:1086:1: warning: restricted blk_opf_t degrades to integer

This is the kernel patch that I applied:

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f1221d11f8e5..10c55f97e02b 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -30,7 +30,7 @@
   * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
   * credit to Christian Biere.
   */
-#define is_signed_type(type)       (((type)(-1)) < (type)1)
+#define is_signed_type(type)       (((__force type)(-1)) <= (__force type)0)
  #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
  #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
  #define type_min(T) ((T)((T)-type_max(T)-(T)1))
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..c74cfa657025 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -9,6 +9,7 @@
  #include <linux/hardirq.h>
  #include <linux/perf_event.h>
  #include <linux/tracepoint.h>
+#include <linux/overflow.h>

  struct trace_array;
  struct array_buffer;
@@ -814,8 +815,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
  extern int trace_remove_event_call(struct trace_event_call *call);
  extern int trace_event_get_offsets(struct trace_event_call *call);

-#define is_signed_type(type)   (((type)(-1)) < (type)1)
-
  int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
  int trace_set_clr_event(const char *system, const char *event, int set);
  int trace_array_set_clr_event(struct trace_array *tr, const char *system,

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 5:09 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> I'm probably doing something wrong but even with sparse commit 658ee8e0f631
> ("unrestricted values are unrestricted even after a cast") I see warnings
> being triggered by users of the is_signed_type() macro, warnings that
> disappear if I change the definition of the is_signed_type() macro into 0:

That's the

> It does require that kernel change to make
>
>   #define is_signed_type(type)   (((type)(-1)) <= (type)0)

part I was talking about.

So your kernel side patch looks fine, except I don't think you need
the '__force' - the sparse patches in my tree should make sparse happy
about casting '-1'.

But I didn't do very much testing.

                     Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Al Viro]

On Tue, Aug 23, 2022 at 04:57:00PM -0700, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Can you try the sparse version at
> >
> >    git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> >
> > which I just set up temporarily with some patches of mine.
> 
> Ugh, and while testing this with sparse, I noticed that sparse itself
> got that whole 'is_signed_type()' check wrong.
> 
> The sparse fix was to remove one line of code, but that one worries
> me, because that one line was clearly very intentional:
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
> 
> Adding Al, since he's actually the original source of that bitwise
> code (and did a lot of other sparse code on the type handling and
> preprocessor side in particular).

Ouch...  That'll take quite a bit of swap-in (and digging through the
old notes).  I'll take a look, but there's an unrelated problem:
	ANY ordered comparisons should spew warnings on bitwise
And we really want that to happen - things like
#define MASK cpu_to_le32(1023)
	if (foo->len > MASK)
		return -EINVAL;
	something(le32_to_cpu(foo->len));
should trigger warnings and I have seen real bugs of that sort.

So I'm not sure how is that supposed to work without sparse getting
loudly unhappy.

Al, going to look through that thread and then try to reconstruct sparse-related
notes...

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 6:49 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That's the
>
> > It does require that kernel change to make
> >
> >   #define is_signed_type(type)   (((type)(-1)) <= (type)0)
>
> part I was talking about.

Side note: I think you could move this into '<linux/compiler.h>' and that would

 (a) make some conceptual sense (unlike "overflow.h" and "trace_events.h")

and

 (b) mean it gets included automatically in both files.

overflow.h already explicitly includes compiler.h, and trace_events.h
gets it from

   linux/ring_buffer.h -> linux/mm.h -> linux/bug.h -> asm/bug.h ->
linux/compiler.h

(it goes other wats too, but those ones are through arch-specific asm
headers, so the above is the first non-arch-specific unconditional
chain I found.

And yes, we should have some tool for sorting out these nasty header
chains. Some automated tool that says "You don't need header X,
because you already got it" or "You don't need headed X, because
nothing you do depends on it".

             Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 7:09 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> I'll take a look, but there's an unrelated problem:
>         ANY ordered comparisons should spew warnings on bitwise

In the general case, absolutely.

But we have two special values that are the same in any bit ordering:
-1 and 0 (all bits set and no bits set). And yes, my patch verifies
that.

See commit 18f17cde ("allow restricted ordered compares with
unrestricted values") in that same tree.

That said, even those are strictly speaking only well-defined in
*unsigned* compares, so I guess we should add that - if you have a
*signed* bitwise thing, even those aren't well-defined to compare
against.

Now, I don't think signed bitwise types make much sense, and I
certainly hope we don't have them in the kernel, but yes, it is
probably worth adding that check too.

Of course, that check then depends on getting the signedness right for
bitwise types, which is exactly the problem I wanted you to look at ;)

                  Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Al Viro]

On Wed, Aug 24, 2022 at 03:09:07AM +0100, Al Viro wrote:
> On Tue, Aug 23, 2022 at 04:57:00PM -0700, Linus Torvalds wrote:
> > On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > Can you try the sparse version at
> > >
> > >    git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> > >
> > > which I just set up temporarily with some patches of mine.
> > 
> > Ugh, and while testing this with sparse, I noticed that sparse itself
> > got that whole 'is_signed_type()' check wrong.
> > 
> > The sparse fix was to remove one line of code, but that one worries
> > me, because that one line was clearly very intentional:
> > 
> >    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
> > 
> > Adding Al, since he's actually the original source of that bitwise
> > code (and did a lot of other sparse code on the type handling and
> > preprocessor side in particular).
> 
> Ouch...  That'll take quite a bit of swap-in (and digging through the
> old notes).

The basic approach was to have them *NOT* treated as integer types;
it's SYM_RESTRICT node refering to the node for underlying type.
MOD_CHAR/MOD_LONG/MOD_UNSIGNED/etc. make no more sense for that than
they do for e.g. pointers.

Any operations like ordered comparisons would trigger unrestrict() on
these suckers, which would warn and convert to underlying type.

Your addition of "ordered comparison with 0 or -1" evades unrestrict().
Which shuts the warning up, but that leaves evaluate_compare() with
something unexpected -
		if (ctype->ctype.modifiers & MOD_UNSIGNED)
			expr->op = modify_for_unsigned(expr->op);
running into SYM_RESTRICT node.

*IF* you want to go that way, I would suggest a new return value for
restricted_binop() ("comparison with magical value"), with
something like
		switch (restricted_binop(op, ctype)) {
		case 1:
		....

		default:
			break;
		case 4:
			// comparison with magic value:
			// quietly go for underlying type, if not fouled
			// if fouled, just return NULL and let the caller
			// deal with that - loudly.
			if (!(lclass & rclass & TYPE_FOULED))
				ctype = ctype->ctype.base_type;
			else
				ctype = NULL;
		}
in restricted_binop_type().

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Al Viro]

On Wed, Aug 24, 2022 at 04:10:25AM +0100, Al Viro wrote:
> *IF* you want to go that way, I would suggest a new return value for
> restricted_binop() ("comparison with magical value"), with
> something like
> 		switch (restricted_binop(op, ctype)) {
> 		case 1:
> 		....
> 
> 		default:
> 			break;
> 		case 4:
> 			// comparison with magic value:
> 			// quietly go for underlying type, if not fouled
> 			// if fouled, just return NULL and let the caller
> 			// deal with that - loudly.
> 			if (!(lclass & rclass & TYPE_FOULED))

 			if (!((lclass | rclass) & TYPE_FOULED))

that is - we don't need both of them being fouled to trigger a warning;
either one should suffice.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/23/22 18:49, Linus Torvalds wrote:
> That's the
> 
>> It does require that kernel change to make
>>
>>    #define is_signed_type(type)   (((type)(-1)) <= (type)0)
> 
> part I was talking about.
> 
> So your kernel side patch looks fine, except I don't think you need
> the '__force' - the sparse patches in my tree should make sparse happy
> about casting '-1'.

Thank you for having provided this feedback. If I change the
is_signed_type() definition into the above (no __force), the sparse
warnings shown in my previous email disappear. Now I'm puzzled about
how this is possible. I guess I should take a closer look at the sparse
patches.

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/23/22 19:11, Linus Torvalds wrote:
> Side note: I think you could move this into '<linux/compiler.h>' and that would
> 
>   (a) make some conceptual sense (unlike "overflow.h" and "trace_events.h")
> 
> and
> 
>   (b) mean it gets included automatically in both files.
> 
> overflow.h already explicitly includes compiler.h, and trace_events.h
> gets it from
> 
>     linux/ring_buffer.h -> linux/mm.h -> linux/bug.h -> asm/bug.h ->
> linux/compiler.h
> 
> (it goes other wats too, but those ones are through arch-specific asm
> headers, so the above is the first non-arch-specific unconditional
> chain I found.

OK, I will look into moving the definition of is_signed_type() into compiler.h.

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Tue, Aug 23, 2022 at 8:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Any operations like ordered comparisons would trigger unrestrict() on
> these suckers, which would warn and convert to underlying type.
>
> Your addition of "ordered comparison with 0 or -1" evades unrestrict().

No. Look. Try this modification to that test, and use
'./test-linearize' to see what sparse turns it into without my patch
to keep the signedness.

    static long test(void)
    {
        return (le32) 0xffffffff;
    }

yes, yes, it warns (twice, actually), but it also then generates

        ret.64      $-1

for that return.

Why? Because it thinks that 'le32' is a signed 32-bit thing due to the
clearing of the MOD_UNSIGNED bit, so when it casts it to 'long' it
will sign-extend it.

So the sign confusion exists and is visible regardless of the added
ordered comparison.

Now, we normally don't *notice* any of this, because we obviously
don't rely on sparse generating any code. And we _do_ cast those
bitwise things in many places, although we use "__force" to show that
it's intentional. Including, very much, those kinds of widening casts
where the signedness matters.

See for example very much the csum code:

    __wsum csum_partial(const void *buff, int len, __wsum wsum)
    {
        unsigned int sum = (__force unsigned int)wsum;

which is *exactly* that kind of code where it's fundamentally
important that 'wsum' is an unsigned type, and casting it to 'unsigned
int' does not sign-extend it.

So no. This has absolutely nothing to do with the new ordered comparisons.

Those bitwise types have always been integers, just with special rules
for warning about mis-using them.

And the sign handling has always been wrong.

It just so happens that me using 'test-linearize' to double-check what
sparse does for that signedness check *uncovered* that pre-existing
bug.

It was not introduced by the new code, and the ordered comparisons are
not AT ALL different from the equality comparisons, except for the
fact that they actually care about the signedness being right.

           Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/23/22 18:49, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 5:09 PM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> I'm probably doing something wrong but even with sparse commit 658ee8e0f631
>> ("unrestricted values are unrestricted even after a cast") I see warnings
>> being triggered by users of the is_signed_type() macro, warnings that
>> disappear if I change the definition of the is_signed_type() macro into 0:
> 
> That's the
> 
>> It does require that kernel change to make
>>
>>    #define is_signed_type(type)   (((type)(-1)) <= (type)0)
> 
> part I was talking about.
> 
> So your kernel side patch looks fine, except I don't think you need
> the '__force' - the sparse patches in my tree should make sparse happy
> about casting '-1'.
> 
> But I didn't do very much testing.

Hi Linus,

Can you take a look at the following report from the kernel test robot:
https://lore.kernel.org/all/202208250433.EVz5pZkB-lkp@intel.com/ ?

Do I see correctly that gcc reports a new warning for the above 
definition of is_signed_type() with W=1? I'm not sure how to get rid of 
that new gcc warning without reintroducing a sparse warning.

The tree that the kernel robot tested is available here: 
https://github.com/bvanassche/linux/tree/tracing

Thanks,

Bart.

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Wed, Aug 24, 2022 at 4:28 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Can you take a look at the following report from the kernel test robot:
> https://lore.kernel.org/all/202208250433.EVz5pZkB-lkp@intel.com/ ?
>
> Do I see correctly that gcc reports a new warning for the above
> definition of is_signed_type() with W=1? I'm not sure how to get rid of
> that new gcc warning without reintroducing a sparse warning.

Indeed. It looks like now gcc recognizes it as a NULL pointer, and
then special-cases that and warns for it.

Oh, how very annoying.

Let me think about this.

               Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Wed, Aug 24, 2022 at 5:30 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Let me think about this.

Actually, thinking about it, that was simple enough.

  -#define is_signed_type(type)   (((type)(-1)) < (type)1)
  +#define is_signed_type(type)   (((type)(-1)) < (__force type)1)

should work.

It looks a bit odd, because we only force one side.

But we only need to force one side, because the '-1' doesn't have any
issues with bitwise types, the same way 0 doesn't.

So only '1' needs to be force-cast to avoid a warning about casting an
integer to a bitwise type.

And since that -1 counts as an unrestricted value after a cast, now
the ordered comparison doesn't warn either.

Now, admittedly I think sparse should also allow a forced cast of an
unrestricted value to be unrestricted, so I think I should do this

 static int restricted_value(struct expression *v, struct symbol *type)
 {
-       if (v->type == EXPR_CAST)
+       if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
                v = v->cast_expression;

in sparse, but even without that the above "is_signed_type()" macro
should make sparse happy (with that current tree of mine).

And since we don't now need to cast 0, gcc won't complain about that
NULL pointer comparison.

Does that solve things for you?

             Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Rasmus Villemoes]

On 25/08/2022 02.40, Linus Torvalds wrote:
> On Wed, Aug 24, 2022 at 5:30 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Let me think about this.
> 
> Actually, thinking about it, that was simple enough.
> 
>   -#define is_signed_type(type)   (((type)(-1)) < (type)1)
>   +#define is_signed_type(type)   (((type)(-1)) < (__force type)1)
> 
> should work.
> 
> It looks a bit odd, because we only force one side.

One can also make the RHS not be a null pointer constant with something like

(((t)(-1)) <= (1 ? (t)0 : (t)0))

In either case, we're a good way into "this needs a comment explaining
why it's written precisely the way it is" territory.

Rasmus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Linus Torvalds]

On Thu, Aug 25, 2022 at 12:57 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> One can also make the RHS not be a null pointer constant with something like
>
> (((t)(-1)) <= (1 ? (t)0 : (t)0))

Oh Gods.

Let's not go there. I'm sure some version of gcc will figure that out
as being NULL in the end and warn about that too.

I do agree that a comment about the exact choice and why (integers vs
pointers with NULL conversions, and compilers vs sparse) for the
particular syntax would not be misplaced.

               Linus

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/24/22 17:40, Linus Torvalds wrote:
> Actually, thinking about it, that was simple enough.
> 
>    -#define is_signed_type(type)   (((type)(-1)) < (type)1)
>    +#define is_signed_type(type)   (((type)(-1)) < (__force type)1)
> 
> should work.
> 
> It looks a bit odd, because we only force one side.
> 
> But we only need to force one side, because the '-1' doesn't have any
> issues with bitwise types, the same way 0 doesn't.
> 
> So only '1' needs to be force-cast to avoid a warning about casting an
> integer to a bitwise type.
> 
> And since that -1 counts as an unrestricted value after a cast, now
> the ordered comparison doesn't warn either.
> 
> Now, admittedly I think sparse should also allow a forced cast of an
> unrestricted value to be unrestricted, so I think I should do this
> 
>   static int restricted_value(struct expression *v, struct symbol *type)
>   {
> -       if (v->type == EXPR_CAST)
> +       if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
>                  v = v->cast_expression;
> 
> in sparse, but even without that the above "is_signed_type()" macro
> should make sparse happy (with that current tree of mine).
> 
> And since we don't now need to cast 0, gcc won't complain about that
> NULL pointer comparison.
> 
> Does that solve things for you?

Yes, thank you! No sparse warnings are triggered by the is_signed_type()
macro and the gcc warning about ordered comparison of a pointer with the
null pointer is gone.

The patch I came up with is available below. If nobody picks it up from
this email I will try to find an appropriate kernel maintainer to send
this kernel patch to.

Thanks,

Bart.


From: Bart Van Assche <bvanassche@acm.org>
Date: Tue, 23 Aug 2022 12:59:25 -0700
Subject: [PATCH] tracing: Define the is_signed_type() macro once

There are two definitions of the is_signed_type() macro: one in
<linux/overflow.h> and a second definition in <linux/trace_events.h>.

As suggested by Linus Torvalds, move the definition of the
is_signed_type() macro into the <linux/compiler.h> header file. Change
the definition of the is_signed_type() macro to make sure that it does
not trigger any sparse warnings with future versions of sparse for
bitwise types. See also:
https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/

Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  include/linux/compiler.h     | 6 ++++++
  include/linux/overflow.h     | 1 -
  include/linux/trace_events.h | 2 --
  3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 01ce94b58b42..7713d7bcdaea 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -239,6 +239,12 @@ static inline void *offset_to_ptr(const int *off)
  /* &a[0] degrades to a pointer: a different type from an array */
  #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

+/*
+ * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
+ * bool and also pointer types.
+ */
+#define is_signed_type(type) (((type)(-1)) < (__force type)1)
+
  /*
   * This is needed in functions which generate the stack canary, see
   * arch/x86/kernel/smpboot.c::start_secondary() for an example.
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index f1221d11f8e5..0eb3b192f07a 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -30,7 +30,6 @@
   * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
   * credit to Christian Biere.
   */
-#define is_signed_type(type)       (((type)(-1)) < (type)1)
  #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
  #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
  #define type_min(T) ((T)((T)-type_max(T)-(T)1))
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index b18759a673c6..8401dec93c15 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -814,8 +814,6 @@ extern int trace_add_event_call(struct trace_event_call *call);
  extern int trace_remove_event_call(struct trace_event_call *call);
  extern int trace_event_get_offsets(struct trace_event_call *call);

-#define is_signed_type(type)	(((type)(-1)) < (type)1)
-
  int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
  int trace_set_clr_event(const char *system, const char *event, int set);
  int trace_array_set_clr_event(struct trace_array *tr, const char *system,

Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Kees Cook]

On Thu, Aug 25, 2022 at 10:39:02AM -0700, Bart Van Assche wrote:
> On 8/24/22 17:40, Linus Torvalds wrote:
> > Actually, thinking about it, that was simple enough.
> > 
> >    -#define is_signed_type(type)   (((type)(-1)) < (type)1)
> >    +#define is_signed_type(type)   (((type)(-1)) < (__force type)1)
> > 
> > should work.
> > 
> > It looks a bit odd, because we only force one side.
> > 
> > But we only need to force one side, because the '-1' doesn't have any
> > issues with bitwise types, the same way 0 doesn't.
> > 
> > So only '1' needs to be force-cast to avoid a warning about casting an
> > integer to a bitwise type.
> > 
> > And since that -1 counts as an unrestricted value after a cast, now
> > the ordered comparison doesn't warn either.
> > 
> > Now, admittedly I think sparse should also allow a forced cast of an
> > unrestricted value to be unrestricted, so I think I should do this
> > 
> >   static int restricted_value(struct expression *v, struct symbol *type)
> >   {
> > -       if (v->type == EXPR_CAST)
> > +       if (v->type == EXPR_CAST || v->type = EXPR_FORCE_CAST)
> >                  v = v->cast_expression;
> > 
> > in sparse, but even without that the above "is_signed_type()" macro
> > should make sparse happy (with that current tree of mine).
> > 
> > And since we don't now need to cast 0, gcc won't complain about that
> > NULL pointer comparison.
> > 
> > Does that solve things for you?
> 
> Yes, thank you! No sparse warnings are triggered by the is_signed_type()
> macro and the gcc warning about ordered comparison of a pointer with the
> null pointer is gone.
> 
> The patch I came up with is available below. If nobody picks it up from
> this email I will try to find an appropriate kernel maintainer to send
> this kernel patch to.
> 
> Thanks,
> 
> Bart.
> 
> 
> From: Bart Van Assche <bvanassche@acm.org>
> Date: Tue, 23 Aug 2022 12:59:25 -0700
> Subject: [PATCH] tracing: Define the is_signed_type() macro once
> 
> There are two definitions of the is_signed_type() macro: one in
> <linux/overflow.h> and a second definition in <linux/trace_events.h>.
> 
> As suggested by Linus Torvalds, move the definition of the
> is_signed_type() macro into the <linux/compiler.h> header file. Change
> the definition of the is_signed_type() macro to make sure that it does
> not trigger any sparse warnings with future versions of sparse for
> bitwise types. See also:
> https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/
> https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/
> 
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good to me; thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook