linux-kernel - Re: [PATCH] tracing: Do not free snapshot if tracer is on cmdline

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: <20221005220856.d2c2db5a499a79874f0c96e3@kernel.org>
Date:   Wed, 5 Oct 2022 22:08:56 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ross Zwisler <zwisler@...nel.org>
Subject: Re: [PATCH] tracing: Do not free snapshot if tracer is on cmdline

On Tue, 4 Oct 2022 18:04:52 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> 
> The ftrace_boot_snapshot and alloc_snapshot cmdline options allocate the
> snapshot buffer at boot up for use later. The ftrace_boot_snapshot in
> particular requires the snapshot to be allocated because it will take a
> snapshot at the end of boot up allowing to see the traces that happened
> during boot so that it's not lost when user space takes over.
> 
> When a tracer is registered (started) there's a path that checks if it
> requires the snapshot buffer or not, and if it does not and it was
> allocated it will do a synchronization and free the snapshot buffer.
> 
> This is only required if the previous tracer was using it for "max
> latency" snapshots, as it needs to make sure all max snapshots are
> complete before freeing. But this is only needed if the previous tracer
> was using the snapshot buffer for latency (like irqoff tracer and
> friends). But it does not make sense to free it, if the previous tracer
> was not using it, and the snapshot was allocated by the cmdline
> parameters. This basically takes away the point of allocating it in the
> first place!
> 
> Note, the allocated snapshot worked fine for just trace events, but fails
> when a tracer is enabled on the cmdline.
> 
> Cc: stable@...r.kernel.org
> Fixes: 55034cd6e6481 ("tracing: Add alloc_snapshot kernel command line parameter")
> Reported-by: Ross Zwisler <zwisler@...nel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>

> ---
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index def721de68a0..871e2b592969 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6432,7 +6432,7 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>  	tr->current_trace = &nop_trace;
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> -	had_max_tr = tr->allocated_snapshot;
> +	had_max_tr = tr->current_trace->use_max_tr;
>  
>  	if (had_max_tr && !t->use_max_tr) {
>  		/*
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>