linux-kernel - Re: [PATCH v3] Add osnoise_trace_is

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] [day] [month] [year] [list]

Message-ID: <142694b4-85de-412b-9016-4ea3c66e726b@stanley.mountain>
Date: Wed, 15 Jan 2025 21:27:48 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Costa Shulyupin <costa.shul@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	Daniel Bristot de Oliveira <bristot@...nel.org>,
	John Kacur <jkacur@...hat.com>,
	"Luis Claudio R. Goncalves" <lgoncalv@...hat.com>,
	Eder Zulian <ezulian@...hat.com>, Tomas Glozar <tglozar@...hat.com>,
	Gabriele Monaco <gmonaco@...hat.com>,
	linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Add osnoise_trace_is_off()

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@...ry=0x418458, trace=trace@...ry=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