linux-kernel - Re: [PATCH] tracing: add trace_seq

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: <20240122175701.3e525404@gandalf.local.home>
Date: Mon, 22 Jan 2024 17:57:01 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Ricardo B. Marliere" <ricardo@...liere.net>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing: add trace_seq_reset function

On Mon, 22 Jan 2024 19:45:41 -0300
"Ricardo B. Marliere" <ricardo@...liere.net> wrote:

> > Perhaps we need a:
> > 
> > 	if (WARN_ON_ONCE(!s->seq.size))
> > 		seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE);
> > 	else
> > 		seq_buf_clear(&s->seq);
> > 
> > 
> > in the trace_seq_reset() to catch any place that doesn't have it
> > initialized yet.  
> 
> But that would be temporary, right? Kind of a "trace_seq_init_or_reset".
> If that's the case it would be best to simply work out all the places
> instead. Would the current tests [1] cover everything or should
> something else be made to make sure no place was missing from the patch?

No it would be permanent. And no, it is *not* a "trace_seq_init_or_reset".
That WARN_ON_ONCE() means this should never happen, but if it does, the
if () statement keeps it from crashing.

If we later make it dynamic, where there is no s->buffer, that would then
change to:

	if (WARN_ON_ONCE(!s->seq.size))
		seq_buf_init(&s->seq, NULL, 0);
	else
		seq_buf_clear(&s->seq);

Where the trace_seq is basically disabled from that point on.

I would try to fix everything, but this will find those places you missed. ;-)

The kernel is like this, because it's not like any other application. If an
application crashes, you may get annoyed, and just restart it. If the
kernel crashes, you can easily lose data and you have to reboot your
machine. Thus, we treat things that could cause harm much more carefully.

We do not want to crash the kernel just because we missed a location that
did not initialize trace_seq. That's why this would be a permanent change.

-- Steve