diff mbox series

[RESEND,v3,1/3] tracing/synthetic: use union instead of casts

Message ID 20230816154928.4171614-2-svens@linux.ibm.com (mailing list archive)
State Accepted
Commit ddeea494a16f32522bce16ee65f191d05d4b8282
Headers show
Series few fixes for synthetic trace events | expand

Commit Message

Sven Schnelle Aug. 16, 2023, 3:49 p.m. UTC
The current code uses a lot of casts to access the fields
member in struct synth_trace_events with different sizes.
This makes the code hard to read, and had already introduced
an endianness bug. Use a union and struct instead.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 include/linux/trace_events.h      | 11 ++++
 kernel/trace/trace.h              |  8 +++
 kernel/trace/trace_events_synth.c | 87 +++++++++++++------------------
 3 files changed, 56 insertions(+), 50 deletions(-)

Comments

kernel test robot Aug. 25, 2023, 8:26 a.m. UTC | #1
Hello,

kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:

commit: e2c9745169808b98f235c09d1366cf8b53ce0d3c ("[PATCH RESEND v3 1/3] tracing/synthetic: use union instead of casts")
url: https://github.com/intel-lab-lkp/linux/commits/Sven-Schnelle/tracing-synthetic-use-union-instead-of-casts/20230817-002758
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6
patch link: https://lore.kernel.org/all/20230816154928.4171614-2-svens@linux.ibm.com/
patch subject: [PATCH RESEND v3 1/3] tracing/synthetic: use union instead of casts

in testcase: boot

compiler: clang-16
test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202308251530.5bd302e0-oliver.sang@intel.com



[   64.204176][   T48] Dumping ftrace buffer:
[   64.204632][   T48] ---------------------------------
[   64.205082][   T48] BUG: unable to handle page fault for address: 001c24ca
[   64.205641][   T48] #PF: supervisor read access in kernel mode
[   64.206115][   T48] #PF: error_code(0x0000) - not-present page
[   64.206588][   T48] *pde = 00000000
[   64.206897][   T48] Oops: 0000 [#1] SMP
[   64.207221][   T48] CPU: 0 PID: 48 Comm: rcu_scale_write Tainted: G                T  6.5.0-rc6-00037-ge2c974516980 #1
[   64.208074][   T48] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[   64.208892][   T48] EIP: string+0x98/0x100
[   64.209237][   T48] Code: 94 c2 75 b4 a1 8c c2 75 b4 40 89 44 24 18 31 d2 eb 0e 47 89 3d 94 c2 75 b4 42 39 54 24 0c 74 32 8b 44 24 08 8d 0c 10 8b 45 08 <0f> b6 04
 10 84 c0 74 2c 8b 74 24 18 01 d6 89 35 8c c2 75 b4 3b 0c
[   64.210761][   T48] EAX: 001c24ca EBX: 000405c2 ECX: b516e406 EDX: 00000000
[   64.211325][   T48] ESI: ffff0a00 EDI: 00005af2 EBP: b600bcd4 ESP: b600bca8
[   64.211876][   T48] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010046
[   64.212468][   T48] CR0: 80050033 CR2: 001c24ca CR3: 08d2a000 CR4: 000406d0
[   64.213022][   T48] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[   64.213577][   T48] DR6: fffe0ff0 DR7: 00000400
[   64.213945][   T48] Call Trace:
[   64.214208][   T48]  ? __die_body+0x69/0xc0
[   64.214546][   T48]  ? __die+0x7e/0x90
[   64.214863][   T48]  ? page_fault_oops+0x22d/0x2b0
[   64.215242][   T48]  ? is_prefetch+0x4a/0x220
[   64.215605][   T48]  ? kernelmode_fixup_or_oops+0xfa/0x140
[   64.216051][   T48]  ? __bad_area_nosemaphore+0x64/0x2a0
[   64.216485][   T48]  ? bad_area_nosemaphore+0x12/0x20
[   64.216899][   T48]  ? do_user_addr_fault+0x650/0x7e0
[   64.217313][   T48]  ? pvclock_clocksource_read_nowd+0x7c/0x230
[   64.217800][   T48]  ? exc_page_fault+0xc5/0x358
[   64.218183][   T48]  ? pvclock_clocksource_read_nowd+0x230/0x230
[   64.218666][   T48]  ? handle_exception+0x14b/0x14b
[   64.219059][   T48]  ? number+0x9b/0x630
[   64.219378][   T48]  ? pvclock_clocksource_read_nowd+0x230/0x230
[   64.219866][   T48]  ? string+0x98/0x100
[   64.220187][   T48]  ? pvclock_clocksource_read_nowd+0x230/0x230
[   64.220656][   T48]  ? string+0x98/0x100
[   64.220986][   T48]  vsnprintf+0x420/0x580
[   64.221323][   T48]  ? vsnprintf+0x3e7/0x580
[   64.221669][   T48]  seq_buf_vprintf+0x79/0xc0
[   64.222029][   T48]  trace_seq_printf+0x35/0xa0
[   64.222400][   T48]  print_synth_event+0x26f/0x300
[   64.222805][   T48]  ? trace_event_raw_event_synth+0x410/0x410
[   64.223272][   T48]  print_trace_fmt+0xfe/0x170
[   64.223651][   T48]  print_trace_line+0x10d/0x1c0
[   64.224046][   T48]  ftrace_dump+0x2fa/0x410
[   64.224407][   T48]  rcu_scale_writer+0x59c/0x640
[   64.224808][   T48]  kthread+0x158/0x170
[   64.225146][   T48]  ? rcu_scale_reader+0x180/0x180
[   64.225559][   T48]  ? kthread_unuse_mm+0x160/0x160
[   64.225968][   T48]  ? kthread_unuse_mm+0x160/0x160
[   64.226379][   T48]  ret_from_fork+0x43/0x70
[   64.226742][   T48]  ret_from_fork_asm+0x12/0x1c
[   64.227130][   T48]  entry_INT80_32+0x108/0x108
[   64.227516][   T48] Modules linked in:
[   64.227832][   T48] CR2: 00000000001c24ca
[   64.228166][   T48] ---[ end trace 0000000000000000 ]---
[   64.228586][   T48] EIP: string+0x98/0x100
[   64.228921][   T48] Code: 94 c2 75 b4 a1 8c c2 75 b4 40 89 44 24 18 31 d2 eb 0e 47 89 3d 94 c2 75 b4 42 39 54 24 0c 74 32 8b 44 24 08 8d 0c 10 8b 45 08 <0f> b6 04
 10 84 c0 74 2c 8b 74 24 18 01 d6 89 35 8c c2 75 b4 3b 0c
[   64.230401][   T48] EAX: 001c24ca EBX: 000405c2 ECX: b516e406 EDX: 00000000
[   64.230918][   T48] ESI: ffff0a00 EDI: 00005af2 EBP: b600bcd4 ESP: b600bca8
[   64.231432][   T48] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010046
[   64.231993][   T48] CR0: 80050033 CR2: 001c24ca CR3: 08d2a000 CR4: 000406d0
[   64.232527][   T48] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[   64.233094][   T48] DR6: fffe0ff0 DR7: 00000400
[   64.233474][   T48] Kernel panic - not syncing: Fatal exception
[   64.234050][   T48] Kernel Offset: disabled



The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230825/202308251530.5bd302e0-oliver.sang@intel.com
diff mbox series

Patch

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 3930e676436c..1e8bbdb8da90 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -59,6 +59,17 @@  int trace_raw_output_prep(struct trace_iterator *iter,
 extern __printf(2, 3)
 void trace_event_printf(struct trace_iterator *iter, const char *fmt, ...);
 
+/* Used to find the offset and length of dynamic fields in trace events */
+struct trace_dynamic_info {
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	u16	offset;
+	u16	len;
+#else
+	u16	len;
+	u16	offset;
+#endif
+};
+
 /*
  * The trace entry - the most basic unit of tracing. This is what
  * is printed in the end as a single line in the trace output, such as:
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index e1edc2197fc8..95956f75bea5 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1295,6 +1295,14 @@  static inline void trace_branch_disable(void)
 /* set ring buffers to default size if not already done so */
 int tracing_update_buffers(void);
 
+union trace_synth_field {
+	u8				as_u8;
+	u16				as_u16;
+	u32				as_u32;
+	u64				as_u64;
+	struct trace_dynamic_info	as_dynamic;
+};
+
 struct ftrace_event_field {
 	struct list_head	link;
 	const char		*name;
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index dd398afc8e25..7fff8235075f 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -127,7 +127,7 @@  static bool synth_event_match(const char *system, const char *event,
 
 struct synth_trace_event {
 	struct trace_entry	ent;
-	u64			fields[];
+	union trace_synth_field	fields[];
 };
 
 static int synth_event_define_fields(struct trace_event_call *call)
@@ -321,19 +321,19 @@  static const char *synth_field_fmt(char *type)
 
 static void print_synth_event_num_val(struct trace_seq *s,
 				      char *print_fmt, char *name,
-				      int size, u64 val, char *space)
+				      int size, union trace_synth_field *val, char *space)
 {
 	switch (size) {
 	case 1:
-		trace_seq_printf(s, print_fmt, name, (u8)val, space);
+		trace_seq_printf(s, print_fmt, name, val->as_u8, space);
 		break;
 
 	case 2:
-		trace_seq_printf(s, print_fmt, name, (u16)val, space);
+		trace_seq_printf(s, print_fmt, name, val->as_u16, space);
 		break;
 
 	case 4:
-		trace_seq_printf(s, print_fmt, name, (u32)val, space);
+		trace_seq_printf(s, print_fmt, name, val->as_u32, space);
 		break;
 
 	default:
@@ -374,36 +374,26 @@  static enum print_line_t print_synth_event(struct trace_iterator *iter,
 		/* parameter values */
 		if (se->fields[i]->is_string) {
 			if (se->fields[i]->is_dynamic) {
-				u32 offset, data_offset;
-				char *str_field;
-
-				offset = (u32)entry->fields[n_u64];
-				data_offset = offset & 0xffff;
-
-				str_field = (char *)entry + data_offset;
+				union trace_synth_field *data = &entry->fields[n_u64];
 
 				trace_seq_printf(s, print_fmt, se->fields[i]->name,
 						 STR_VAR_LEN_MAX,
-						 str_field,
+						 (char *)entry + data->as_dynamic.offset,
 						 i == se->n_fields - 1 ? "" : " ");
 				n_u64++;
 			} else {
 				trace_seq_printf(s, print_fmt, se->fields[i]->name,
 						 STR_VAR_LEN_MAX,
-						 (char *)&entry->fields[n_u64],
+						 (char *)&entry->fields[n_u64].as_u64,
 						 i == se->n_fields - 1 ? "" : " ");
 				n_u64 += STR_VAR_LEN_MAX / sizeof(u64);
 			}
 		} else if (se->fields[i]->is_stack) {
-			u32 offset, data_offset, len;
 			unsigned long *p, *end;
+			union trace_synth_field *data = &entry->fields[n_u64];
 
-			offset = (u32)entry->fields[n_u64];
-			data_offset = offset & 0xffff;
-			len = offset >> 16;
-
-			p = (void *)entry + data_offset;
-			end = (void *)p + len - (sizeof(long) - 1);
+			p = (void *)entry + data->as_dynamic.offset;
+			end = (void *)p + data->as_dynamic.len - (sizeof(long) - 1);
 
 			trace_seq_printf(s, "%s=STACK:\n", se->fields[i]->name);
 
@@ -419,13 +409,13 @@  static enum print_line_t print_synth_event(struct trace_iterator *iter,
 			print_synth_event_num_val(s, print_fmt,
 						  se->fields[i]->name,
 						  se->fields[i]->size,
-						  entry->fields[n_u64],
+						  &entry->fields[n_u64],
 						  space);
 
 			if (strcmp(se->fields[i]->type, "gfp_t") == 0) {
 				trace_seq_puts(s, " (");
 				trace_print_flags_seq(s, "|",
-						      entry->fields[n_u64],
+						      entry->fields[n_u64].as_u64,
 						      __flags);
 				trace_seq_putc(s, ')');
 			}
@@ -454,21 +444,16 @@  static unsigned int trace_string(struct synth_trace_event *entry,
 	int ret;
 
 	if (is_dynamic) {
-		u32 data_offset;
+		union trace_synth_field *data = &entry->fields[*n_u64];
 
-		data_offset = struct_size(entry, fields, event->n_u64);
-		data_offset += data_size;
-
-		len = fetch_store_strlen((unsigned long)str_val);
-
-		data_offset |= len << 16;
-		*(u32 *)&entry->fields[*n_u64] = data_offset;
+		data->as_dynamic.offset = struct_size(entry, fields, event->n_u64) + data_size;
+		data->as_dynamic.len = fetch_store_strlen((unsigned long)str_val);
 
 		ret = fetch_store_string((unsigned long)str_val, &entry->fields[*n_u64], entry);
 
 		(*n_u64)++;
 	} else {
-		str_field = (char *)&entry->fields[*n_u64];
+		str_field = (char *)&entry->fields[*n_u64].as_u64;
 
 #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 		if ((unsigned long)str_val < TASK_SIZE)
@@ -492,6 +477,7 @@  static unsigned int trace_stack(struct synth_trace_event *entry,
 				 unsigned int data_size,
 				 unsigned int *n_u64)
 {
+	union trace_synth_field *data = &entry->fields[*n_u64];
 	unsigned int len;
 	u32 data_offset;
 	void *data_loc;
@@ -515,8 +501,9 @@  static unsigned int trace_stack(struct synth_trace_event *entry,
 	memcpy(data_loc, stack, len);
 
 	/* Fill in the field that holds the offset/len combo */
-	data_offset |= len << 16;
-	*(u32 *)&entry->fields[*n_u64] = data_offset;
+
+	data->as_dynamic.offset = data_offset;
+	data->as_dynamic.len = len;
 
 	(*n_u64)++;
 
@@ -592,19 +579,19 @@  static notrace void trace_event_raw_event_synth(void *__data,
 
 			switch (field->size) {
 			case 1:
-				*(u8 *)&entry->fields[n_u64] = (u8)val;
+				entry->fields[n_u64].as_u8 = (u8)val;
 				break;
 
 			case 2:
-				*(u16 *)&entry->fields[n_u64] = (u16)val;
+				entry->fields[n_u64].as_u16 = (u16)val;
 				break;
 
 			case 4:
-				*(u32 *)&entry->fields[n_u64] = (u32)val;
+				entry->fields[n_u64].as_u32 = (u32)val;
 				break;
 
 			default:
-				entry->fields[n_u64] = val;
+				entry->fields[n_u64].as_u64 = val;
 				break;
 			}
 			n_u64++;
@@ -1791,19 +1778,19 @@  int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...)
 
 			switch (field->size) {
 			case 1:
-				*(u8 *)&state.entry->fields[n_u64] = (u8)val;
+				state.entry->fields[n_u64].as_u8 = (u8)val;
 				break;
 
 			case 2:
-				*(u16 *)&state.entry->fields[n_u64] = (u16)val;
+				state.entry->fields[n_u64].as_u16 = (u16)val;
 				break;
 
 			case 4:
-				*(u32 *)&state.entry->fields[n_u64] = (u32)val;
+				state.entry->fields[n_u64].as_u32 = (u32)val;
 				break;
 
 			default:
-				state.entry->fields[n_u64] = val;
+				state.entry->fields[n_u64].as_u64 = val;
 				break;
 			}
 			n_u64++;
@@ -1884,19 +1871,19 @@  int synth_event_trace_array(struct trace_event_file *file, u64 *vals,
 
 			switch (field->size) {
 			case 1:
-				*(u8 *)&state.entry->fields[n_u64] = (u8)val;
+				state.entry->fields[n_u64].as_u8 = (u8)val;
 				break;
 
 			case 2:
-				*(u16 *)&state.entry->fields[n_u64] = (u16)val;
+				state.entry->fields[n_u64].as_u16 = (u16)val;
 				break;
 
 			case 4:
-				*(u32 *)&state.entry->fields[n_u64] = (u32)val;
+				state.entry->fields[n_u64].as_u32 = (u32)val;
 				break;
 
 			default:
-				state.entry->fields[n_u64] = val;
+				state.entry->fields[n_u64].as_u64 = val;
 				break;
 			}
 			n_u64++;
@@ -2031,19 +2018,19 @@  static int __synth_event_add_val(const char *field_name, u64 val,
 	} else {
 		switch (field->size) {
 		case 1:
-			*(u8 *)&trace_state->entry->fields[field->offset] = (u8)val;
+			trace_state->entry->fields[field->offset].as_u8 = (u8)val;
 			break;
 
 		case 2:
-			*(u16 *)&trace_state->entry->fields[field->offset] = (u16)val;
+			trace_state->entry->fields[field->offset].as_u16 = (u16)val;
 			break;
 
 		case 4:
-			*(u32 *)&trace_state->entry->fields[field->offset] = (u32)val;
+			trace_state->entry->fields[field->offset].as_u32 = (u32)val;
 			break;
 
 		default:
-			trace_state->entry->fields[field->offset] = val;
+			trace_state->entry->fields[field->offset].as_u64 = val;
 			break;
 		}
 	}