linux-kernel - Re: [PATCH] tracing: Add __cpumask to denote a trace event field that is a cpumask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 14 Oct 2022 18:21:41 +0100
From:   Valentin Schneider <vschneid@...hat.com>
To:     Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>,
        Douglas RAILLARD <douglas.raillard@....com>
Subject: Re: [PATCH] tracing: Add __cpumask to denote a trace event field
 that is a cpumask_t

+Cc Douglas

On 14/10/22 08:04, Steven Rostedt wrote:
> From: Steven Rostedt (Google) <rostedt@...dmis.org>
>
> The trace events have a __bitmask field that can be used for anything
> that requires bitmasks. Although currently it is only used for CPU
> masks, it could be used in the future for any type of bitmasks.
>
> There is some user space tooling that wants to know if a field is a CPU
> mask and not just some random unsigned long bitmask. Introduce
> "__cpumask()" helper functions that work the same as the current
> __bitmask() helpers but displays in the format file:
>
>   field:__data_loc cpumask_t *[] mask;    offset:36;      size:4; signed:0;
>
> Instead of:
>
>   field:__data_loc unsigned long[] mask;  offset:32;      size:4; signed:0;
>
> The main difference is the type. Instead of "unsigned long" it is
> "cpumask_t *". Note, this type field needs to be a real type in the
> __dynamic_array() logic that both __cpumask and__bitmask use, but the
> comparison field requires it to be a scalar type whereas cpumask_t is a
> structure (non-scalar). But everything works when making it a pointer.
>
> Requested-by: Valentin Schneider <vschneid@...hat.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Thanks for spinning this out so quickly! I gave it a test against my IPI
tracepoints, it's working OK with one (expected) change:

  [ipi:ipi_send_cpumask] function __get_cpumask not defined
  CPU 0 is empty
  CPU 1 is empty
  CPU 3 is empty
  cpus=4
            echo-173   [002]    11.859745: ipi_send_cpumask:     [FAILED TO PARSE] cpumask=ARRAY[02, 00, 00, 00, 00, 00, 00, 00] callsite=0xffffffff81121013

so libtraceevent is going to need updating - I'm happy to do that (if you
haven't done it already!)

Lastly, given all cpumasks have a (usable) size of nr_cpumask_bits, we can
factor out the size argument, see below. Regardless:

Reviewed-by: Valentin Schneider <vschneid@...hat.com>

---
diff --git a/include/trace/stages/stage1_struct_define.h b/include/trace/stages/stage1_struct_define.h
index e1a9b8462b418..b6a9e60755fe0 100644
--- a/include/trace/stages/stage1_struct_define.h
+++ b/include/trace/stages/stage1_struct_define.h
@@ -33,7 +33,7 @@
 #define __bitmask(item, nr_bits) __dynamic_array(char, item, -1)
 
 #undef __cpumask
-#define __cpumask(item, nr_bits) __dynamic_array(char, item, -1)
+#define __cpumask(item) __dynamic_array(char, item, -1)
 
 #undef __sockaddr
 #define __sockaddr(field, len) __dynamic_array(u8, field, len)
diff --git a/include/trace/stages/stage2_data_offsets.h b/include/trace/stages/stage2_data_offsets.h
index 4f096c30303b9..2c3838f5ee892 100644
--- a/include/trace/stages/stage2_data_offsets.h
+++ b/include/trace/stages/stage2_data_offsets.h
@@ -39,7 +39,7 @@
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
 #undef __cpumask
-#define __cpumask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
+#define __cpumask(item) __dynamic_array(unsigned long, item, -1)
 
 #undef __sockaddr
 #define __sockaddr(field, len) __dynamic_array(u8, field, len)
diff --git a/include/trace/stages/stage4_event_fields.h b/include/trace/stages/stage4_event_fields.h
index 11cf5550494d2..52ccf0b9b1765 100644
--- a/include/trace/stages/stage4_event_fields.h
+++ b/include/trace/stages/stage4_event_fields.h
@@ -47,7 +47,7 @@
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
 #undef __cpumask
-#define __cpumask(item, nr_bits) __dynamic_array(cpumask_t *, item, -1)
+#define __cpumask(item) __dynamic_array(cpumask_t *, item, -1)
 
 #undef __sockaddr
 #define __sockaddr(field, len) __dynamic_array(u8, field, len)
diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
index 0d9386b0d320c..b3934b74c72d0 100644
--- a/include/trace/stages/stage5_get_offsets.h
+++ b/include/trace/stages/stage5_get_offsets.h
@@ -83,7 +83,7 @@
 					 __bitmask_size_in_longs(nr_bits))
 
 #undef __cpumask
-#define __cpumask(item, nr_bits) __bitmask(item, nr_bits)
+#define __cpumask(item) __bitmask(item, nr_cpumask_bits)
 
 #undef __rel_bitmask
 #define __rel_bitmask(item, nr_bits) __rel_dynamic_array(unsigned long, item,	\
diff --git a/include/trace/stages/stage6_event_callback.h b/include/trace/stages/stage6_event_callback.h
index 76774cb9acc89..e12e541fef42b 100644
--- a/include/trace/stages/stage6_event_callback.h
+++ b/include/trace/stages/stage6_event_callback.h
@@ -58,14 +58,14 @@
 	memcpy(__get_bitmask(dst), (src), __bitmask_size_in_bytes(nr_bits))
 
 #undef __cpumask
-#define __cpumask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
+#define __cpumask(item) __dynamic_array(unsigned long, item, -1)
 
 #undef __get_cpumask
 #define __get_cpumask(field) (char *)__get_dynamic_array(field)
 
 #undef __assign_cpumask
-#define __assign_cpumask(dst, src, nr_bits)					\
-	memcpy(__get_cpumask(dst), (src), __bitmask_size_in_bytes(nr_bits))
+#define __assign_cpumask(dst, src)					\
+	memcpy(__get_cpumask(dst), (src), __bitmask_size_in_bytes(nr_cpumask_bits))
 
 #undef __sockaddr
 #define __sockaddr(field, len) __dynamic_array(u8, field, len)