| Message ID | 20250115171359.1954533-1-costa.shul@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Add osnoise_trace_is_off() | expand |
On Wed, Jan 15, 2025 at 07:11:59PM +0200, Costa Shulyupin wrote: > The usage of trace_is_off() confusing, which requires a detailed > explanation. > > Let's modify the source code by moving the first > member, `trace`, of the `osnoise_tool` structure to the second position: > > struct osnoise_tool { > - struct trace_instance trace; > struct osnoise_context *context; > + struct trace_instance trace; > > A correct program would work properly after this change, > but this one does not. > > Then, run the program under gdb to observe the behavior. > > gdb -q --args ./rtla osnoise -D -d 2 -T 10000 -q > > Program received signal SIGSEGV, Segmentation fault. > 0x000000000040298f in trace_is_off (tool=tool@entry=0x418458, trace=trace@entry=0x8) at src/trace.c:538 > 538 if (trace && !tracefs_trace_is_on(trace->inst)) > ... > > The program checks if trace, which has a value of 8, is not null, > and then crashes when it attempts to dereference trace->inst. > > It happens because trace_is_off() is called as: > trace_is_off(&tool->trace, &record->trace) > > Where `record` is NULL. Expression `&record->trace` returns offset of > member `trace`, which is 8. The original code accidentally works because > offset of `record->trace` is zero. > > Expanded wrong instructions are: > record = NULL; > if (&record->trace && !tracefs_trace_is_on(record->trace.inst)) > return 1; > > The correct instructions are: > record = NULL; > if (record && !tracefs_trace_is_on(record->trace.inst)) > return 1; > > Refactor `trace_is_off` to `osnoise_trace_is_off` and move it to > osnoise.c because it instead of `struct trace_instance` accesses `struct > osnoise_tool`, which is out of the scope of trace.c. > This commit message is so confusing. It takes five paragraphs to explain how NULL + sizeof(void *) equals 8. Everyone knows that already, but it's irrelevant because the actual math in the code is "NULL plus zero". And then there is the crash dump which can't happen unless you change the math from zero to an eight. It would be easy get the impression that this change is some kind of complicated fix to a crashing bug. Earlier I provided you with a good commit message which you could just copy and paste but here is an updated version: This patch is just a clean up and does not affect the behavior. Consider this code: if (trace_is_off(&tool->trace, &record->trace)) The "record" pointer can be NULL in this code. If you're new to the code then it looks like a potential NULL dereference. It turns out it's fine because ->trace is the first element of the record and the trace_is_off() function has a NULL check. So it's fine. But it does look sketchy when you're not super familiar with the code. Refactor the trace_is_off() function to take a record pointer instead of the trace pointer and rename it to osnoise_trace_is_off(). You need to add a subsystem prefix to the subject as well. regards, dan carpenter
diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c index 245e9344932bc..fcfaaff6ea164 100644 --- a/tools/tracing/rtla/src/osnoise.c +++ b/tools/tracing/rtla/src/osnoise.c @@ -1079,6 +1079,22 @@ struct osnoise_tool *osnoise_init_trace_tool(char *tracer) return NULL; } +bool osnoise_trace_is_off(struct osnoise_tool *tool, struct osnoise_tool *record) +{ + /* + * The tool instance is always present, it is the one used to collect + * data. + */ + if (!tracefs_trace_is_on(tool->trace.inst)) + return true; + + /* + * The trace record instance is only enabled when -t is set. IOW, when the system + * is tracing. + */ + return record && !tracefs_trace_is_on(record->trace.inst); +} + static void osnoise_usage(int err) { int i; diff --git a/tools/tracing/rtla/src/osnoise.h b/tools/tracing/rtla/src/osnoise.h index 555f4f4903cc2..1dc188baddef9 100644 --- a/tools/tracing/rtla/src/osnoise.h +++ b/tools/tracing/rtla/src/osnoise.h @@ -104,6 +104,7 @@ struct osnoise_tool { void osnoise_destroy_tool(struct osnoise_tool *top); struct osnoise_tool *osnoise_init_tool(char *tool_name); struct osnoise_tool *osnoise_init_trace_tool(char *tracer); +bool osnoise_trace_is_off(struct osnoise_tool *tool, struct osnoise_tool *record); int osnoise_hist_main(int argc, char *argv[]); int osnoise_top_main(int argc, char **argv); diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c index 214e2c93fde01..f250f999a4eee 100644 --- a/tools/tracing/rtla/src/osnoise_hist.c +++ b/tools/tracing/rtla/src/osnoise_hist.c @@ -970,7 +970,7 @@ int osnoise_hist_main(int argc, char *argv[]) goto out_hist; } - if (trace_is_off(&tool->trace, &record->trace)) + if (osnoise_trace_is_off(tool, record)) break; } @@ -980,7 +980,7 @@ int osnoise_hist_main(int argc, char *argv[]) return_value = 0; - if (trace_is_off(&tool->trace, &record->trace)) { + if (osnoise_trace_is_off(tool, record)) { printf("rtla osnoise hit stop tracing\n"); if (params->trace_output) { printf(" Saving trace to %s\n", params->trace_output); diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c index 45647495ce3bd..6d50653ae224c 100644 --- a/tools/tracing/rtla/src/osnoise_top.c +++ b/tools/tracing/rtla/src/osnoise_top.c @@ -801,7 +801,7 @@ int osnoise_top_main(int argc, char **argv) if (!params->quiet) osnoise_print_stats(params, tool); - if (trace_is_off(&tool->trace, &record->trace)) + if (osnoise_trace_is_off(tool, record)) break; } @@ -810,7 +810,7 @@ int osnoise_top_main(int argc, char **argv) return_value = 0; - if (trace_is_off(&tool->trace, &record->trace)) { + if (osnoise_trace_is_off(tool, record)) { printf("osnoise hit stop tracing\n"); if (params->trace_output) { printf(" Saving trace to %s\n", params->trace_output); diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c index 4403cc4eba302..ddb833ce89d01 100644 --- a/tools/tracing/rtla/src/timerlat_hist.c +++ b/tools/tracing/rtla/src/timerlat_hist.c @@ -1342,7 +1342,7 @@ int timerlat_hist_main(int argc, char *argv[]) goto out_hist; } - if (trace_is_off(&tool->trace, &record->trace)) + if (osnoise_trace_is_off(tool, record)) break; /* is there still any user-threads ? */ @@ -1363,7 +1363,7 @@ int timerlat_hist_main(int argc, char *argv[]) return_value = 0; - if (trace_is_off(&tool->trace, &record->trace)) { + if (osnoise_trace_is_off(tool, record)) { printf("rtla timerlat hit stop tracing\n"); if (!params->no_aa) diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c index 059b468981e4d..9a707c42bb1ac 100644 --- a/tools/tracing/rtla/src/timerlat_top.c +++ b/tools/tracing/rtla/src/timerlat_top.c @@ -1093,7 +1093,7 @@ int timerlat_top_main(int argc, char *argv[]) while (!stop_tracing) { sleep(params->sleep_time); - if (params->aa_only && !trace_is_off(&top->trace, &record->trace)) + if (params->aa_only && !osnoise_trace_is_off(top, record)) continue; retval = tracefs_iterate_raw_events(trace->tep, @@ -1110,7 +1110,7 @@ int timerlat_top_main(int argc, char *argv[]) if (!params->quiet) timerlat_print_stats(params, top); - if (trace_is_off(&top->trace, &record->trace)) + if (osnoise_trace_is_off(top, record)) break; /* is there still any user-threads ? */ @@ -1131,7 +1131,7 @@ int timerlat_top_main(int argc, char *argv[]) return_value = 0; - if (trace_is_off(&top->trace, &record->trace)) { + if (osnoise_trace_is_off(top, record)) { printf("rtla timerlat hit stop tracing\n"); if (!params->no_aa) diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index 170a706248abf..6e24649857dd8 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -522,25 +522,6 @@ void trace_events_destroy(struct trace_instance *instance, trace_events_free(events); } -int trace_is_off(struct trace_instance *tool, struct trace_instance *trace) -{ - /* - * The tool instance is always present, it is the one used to collect - * data. - */ - if (!tracefs_trace_is_on(tool->inst)) - return 1; - - /* - * The trace instance is only enabled when -t is set. IOW, when the system - * is tracing. - */ - if (trace && !tracefs_trace_is_on(trace->inst)) - return 1; - - return 0; -} - /* * trace_set_buffer_size - set the per-cpu tracing buffer size. */ diff --git a/tools/tracing/rtla/src/trace.h b/tools/tracing/rtla/src/trace.h index c7c92dc9a18a6..1ddb51cdaf97c 100644 --- a/tools/tracing/rtla/src/trace.h +++ b/tools/tracing/rtla/src/trace.h @@ -47,5 +47,4 @@ int trace_events_enable(struct trace_instance *instance, int trace_event_add_filter(struct trace_events *event, char *filter); int trace_event_add_trigger(struct trace_events *event, char *trigger); -int trace_is_off(struct trace_instance *tool, struct trace_instance *trace); int trace_set_buffer_size(struct trace_instance *trace, int size);
The usage of trace_is_off() confusing, which requires a detailed explanation. Let's modify the source code by moving the first member, `trace`, of the `osnoise_tool` structure to the second position: struct osnoise_tool { - struct trace_instance trace; struct osnoise_context *context; + struct trace_instance trace; A correct program would work properly after this change, but this one does not. Then, run the program under gdb to observe the behavior. gdb -q --args ./rtla osnoise -D -d 2 -T 10000 -q Program received signal SIGSEGV, Segmentation fault. 0x000000000040298f in trace_is_off (tool=tool@entry=0x418458, trace=trace@entry=0x8) at src/trace.c:538 538 if (trace && !tracefs_trace_is_on(trace->inst)) ... The program checks if trace, which has a value of 8, is not null, and then crashes when it attempts to dereference trace->inst. It happens because trace_is_off() is called as: trace_is_off(&tool->trace, &record->trace) Where `record` is NULL. Expression `&record->trace` returns offset of member `trace`, which is 8. The original code accidentally works because offset of `record->trace` is zero. Expanded wrong instructions are: record = NULL; if (&record->trace && !tracefs_trace_is_on(record->trace.inst)) return 1; The correct instructions are: record = NULL; if (record && !tracefs_trace_is_on(record->trace.inst)) return 1; Refactor `trace_is_off` to `osnoise_trace_is_off` and move it to osnoise.c because it instead of `struct trace_instance` accesses `struct osnoise_tool`, which is out of the scope of trace.c. Signed-off-by: Costa Shulyupin <costa.shul@redhat.com> --- v3: - Dot't call a bug - return boolean expression v2: - Refactor trace_is_off() to osnoise_trace_is_off() - Write detailed explanation --- tools/tracing/rtla/src/osnoise.c | 16 ++++++++++++++++ tools/tracing/rtla/src/osnoise.h | 1 + tools/tracing/rtla/src/osnoise_hist.c | 4 ++-- tools/tracing/rtla/src/osnoise_top.c | 4 ++-- tools/tracing/rtla/src/timerlat_hist.c | 4 ++-- tools/tracing/rtla/src/timerlat_top.c | 6 +++--- tools/tracing/rtla/src/trace.c | 19 ------------------- tools/tracing/rtla/src/trace.h | 1 - 8 files changed, 26 insertions(+), 29 deletions(-)