| Message ID | 20250115081157.1274398-1-costa.shul@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Fix bug and add osnoise_trace_is_off() | expand |
You need an "rtla: " subsystem prefix in the subject. You're going to need to remove the words "fix" and "bug" from the subject because this is just a cleanup. On Wed, Jan 15, 2025 at 10:09:56AM +0200, Costa Shulyupin wrote: > The usage of trace_is_off() contains a small and elusive > bug that requires a detailed explanation. > > To expose the bug, 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. No... You introduced a bug by changing the order. You have to undestand that to the original authors this stuff was really easy and they knew the order of the struct members because they chose it deliberately. In the end, they get so used to the code that "&record->trace" just becomes an idiom for casting "record" and they forget how it looks to a newcomer. I *personally* am not a fan of code which assumes we know the order of the struct members so I don't have a problem with you re-writing the code. But the commit message must say that it is just a cleanup and not a fix. Which reminds me that I had intended to create a container_of_first() for code like this which assumes that container_of() is just a cast. There is lots of code like this: struct something *member = container_of(p, struct foo, first_member); if (IS_ERR(member)) { Which relies on the face that "first_member" is the first member of foo struct. It's a quite common thing. regards, dan carpenter
On Wed, Jan 15, 2025 at 10:09:56AM +0200, Costa Shulyupin wrote: > diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c > index 245e9344932bc..20275642a74cd 100644 > --- a/tools/tracing/rtla/src/osnoise.c > +++ b/tools/tracing/rtla/src/osnoise.c > @@ -1079,6 +1079,25 @@ struct osnoise_tool *osnoise_init_trace_tool(char *tracer) > return NULL; > } > > +int 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 1; > + > + /* > + * The trace record instance is only enabled when -t is set. IOW, when the system > + * is tracing. > + */ > + if (record && !tracefs_trace_is_on(record->trace.inst)) > + return 1; > + > + return 0; > +} Also this function should be declared as type bool and return true/false instead of 1/0. regards, dan carpenter
On Wed, 15 Jan 2025 12:02:00 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > You introduced a bug by changing the order. > > You have to undestand that to the original authors this stuff was really > easy and they knew the order of the struct members because they chose it > deliberately. In the end, they get so used to the code that > "&record->trace" just becomes an idiom for casting "record" and they > forget how it looks to a newcomer. > > I *personally* am not a fan of code which assumes we know the order of > the struct members so I don't have a problem with you re-writing the > code. But the commit message must say that it is just a cleanup and not > a fix. > > Which reminds me that I had intended to create a container_of_first() > for code like this which assumes that container_of() is just a cast. > There is lots of code like this: > > struct something *member = container_of(p, struct foo, first_member); > > if (IS_ERR(member)) { > > Which relies on the face that "first_member" is the first member of > foo struct. It's a quite common thing. I agree that this is not a bug, but I will happily take a clean up to make it more robust. -- Steve
diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c index 245e9344932bc..20275642a74cd 100644 --- a/tools/tracing/rtla/src/osnoise.c +++ b/tools/tracing/rtla/src/osnoise.c @@ -1079,6 +1079,25 @@ struct osnoise_tool *osnoise_init_trace_tool(char *tracer) return NULL; } +int 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 1; + + /* + * The trace record instance is only enabled when -t is set. IOW, when the system + * is tracing. + */ + if (record && !tracefs_trace_is_on(record->trace.inst)) + return 1; + + return 0; +} + 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..646ee314dbaef 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); +int 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() contains a small and elusive bug that requires a detailed explanation. To expose the bug, 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)) ... #1 0x0000000000407db4 in osnoise_top_main (argc=argc@entry=7, argv=argv@entry=0x7fffffffe350) at src/osnoise_top.c:804 ... 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 osnoise_top_main() calls trace_is_off() 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` and fix 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> --- v2: - Refactor trace_is_off() to osnoise_trace_is_off() - Write detailed explanation v1: - https://lore.kernel.org/lkml/20250109211358.2619367-1-costa.shul@redhat.com/ --- tools/tracing/rtla/src/osnoise.c | 19 +++++++++++++++++++ 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, 29 insertions(+), 29 deletions(-)