[PATCH v4] tools/rtla: Add osnoise_trace_is_off()

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 <redacted>

---
v4:
- Add prefix to the subject

v3:
- Dot't call a bug
- return boolean expression

v2:
- Refactor trace_is_off() to osnoise_trace_is_off()
- Write detailed explanation

Signed-off-by: Costa Shulyupin <redacted>
---
 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(-)

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);

-- 
2.47.0

On Wed, Jan 15, 2025 at 07:58:30PM +0200, Costa Shulyupin wrote:

The original code accidentally works because offset of
`record->trace` is zero.

It doesn't "accidentally work".  The people who write this kind of
code 100% understand what they are doing.  They don't see it as
anything complicated.

regards,
dan carpenter

On Wed, 15 Jan 2025 21:40:40 +0300
Dan Carpenter [off-list ref] wrote:

On Wed, Jan 15, 2025 at 07:58:30PM +0200, Costa Shulyupin wrote:

quoted

The original code accidentally works because offset of
`record->trace` is zero.  

It doesn't "accidentally work".  The people who write this kind of
code 100% understand what they are doing.  They don't see it as
anything complicated.

I'm taking this patch but I replaced the change log with the following text:

    tools/rtla: Add osnoise_trace_is_off()
    
    All of the users of trace_is_off() passes in &record->trace as the second
    parameter, where record is a pointer to a struct osnoise_tool. This record
    could be NULL and there is a hidden dependency that the trace field is the
    first field to allow &record->trace to work with a NULL record pointer.
    
    In order to make this code a bit more robust, as record shouldn't be
    dereferenced if it is NULL, even if the code does work, create a new
    function called osnoise_trace_is_off() that takes the pointer to a
    struct osnoise_tool as its second parameter. This way it can properly test
    if it is NULL before it dereferences it.
    
    The old function trace_is_off() is removed and the function
    osnoise_trace_is_off() is added into osnoise.c which is what the
    struct osnoise_tool is associated with.


-- Steve

On Thu, Jan 16, 2025 at 07:34:32PM -0500, Steven Rostedt wrote:

On Wed, 15 Jan 2025 21:40:40 +0300
Dan Carpenter [off-list ref] wrote:

quoted

On Wed, Jan 15, 2025 at 07:58:30PM +0200, Costa Shulyupin wrote:

quoted

The original code accidentally works because offset of
`record->trace` is zero.  

It doesn't "accidentally work".  The people who write this kind of
code 100% understand what they are doing.  They don't see it as
anything complicated.

I'm taking this patch but I replaced the change log with the following text:

    tools/rtla: Add osnoise_trace_is_off()
    
    All of the users of trace_is_off() passes in &record->trace as the second
    parameter, where record is a pointer to a struct osnoise_tool. This record
    could be NULL and there is a hidden dependency that the trace field is the
    first field to allow &record->trace to work with a NULL record pointer.
    
    In order to make this code a bit more robust, as record shouldn't be
    dereferenced if it is NULL, even if the code does work, create a new
    function called osnoise_trace_is_off() that takes the pointer to a
    struct osnoise_tool as its second parameter. This way it can properly test
    if it is NULL before it dereferences it.
    
    The old function trace_is_off() is removed and the function
    osnoise_trace_is_off() is added into osnoise.c which is what the
    struct osnoise_tool is associated with.

Thanks!

regards,
dan carpenter