linux-kernel - Re: [PATCH v2] Fix bug and 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ee1e1a7-f0b3-4062-97d9-45a342d0ca21@stanley.mountain>
Date: Wed, 15 Jan 2025 12:02:00 +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 v2] Fix bug and add osnoise_trace_is_off()

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