| 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
| ||
|
Message-ID: <20260129150745.1f9ab283@gandalf.local.home>
Date: Thu, 29 Jan 2026 15:07:45 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/4] tracing: Make the backup instance non-reusable
On Wed, 28 Jan 2026 09:09:56 +0900
"Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> @@ -10632,17 +10658,23 @@ static __init void create_trace_instances(struct dentry *d_tracer)
> static void
> init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
> {
> + umode_t writable_mode = TRACE_MODE_WRITE;
> + bool readonly = trace_array_is_readonly(tr);
> int cpu;
>
> + if (readonly)
> + writable_mode = TRACE_MODE_READ;
> +
> trace_create_file("available_tracers", TRACE_MODE_READ, d_tracer,
> - tr, &show_traces_fops);
> + tr, &show_traces_fops);
>
> - trace_create_file("current_tracer", TRACE_MODE_WRITE, d_tracer,
> - tr, &set_tracer_fops);
> + trace_create_file("current_tracer", writable_mode, d_tracer,
> + tr, &set_tracer_fops);
>
> - trace_create_file("tracing_cpumask", TRACE_MODE_WRITE, d_tracer,
> + trace_create_file("tracing_cpumask", writable_mode, d_tracer,
> tr, &tracing_cpumask_fops);
>
> + /* Options are used for changing print-format even for readonly instance. */
> trace_create_file("trace_options", TRACE_MODE_WRITE, d_tracer,
> tr, &tracing_iter_fops);
>
> @@ -10652,27 +10684,35 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
> trace_create_file("trace_pipe", TRACE_MODE_READ, d_tracer,
> tr, &tracing_pipe_fops);
>
> - trace_create_file("buffer_size_kb", TRACE_MODE_WRITE, d_tracer,
> + trace_create_file("buffer_size_kb", writable_mode, d_tracer,
> tr, &tracing_entries_fops);
>
> trace_create_file("buffer_total_size_kb", TRACE_MODE_READ, d_tracer,
> tr, &tracing_total_entries_fops);
>
> - trace_create_file("free_buffer", 0200, d_tracer,
> - tr, &tracing_free_buffer_fops);
> + if (!readonly) {
> + trace_create_file("free_buffer", 0200, d_tracer,
> + tr, &tracing_free_buffer_fops);
Hmm, why remove the free_buffer. It just shrinks the buffer down to a
minimum. Perhaps its useless, but I it doesn't write to the buffer. Sure it
removes data but so does trace_pipe.
>
> - trace_create_file("trace_marker", 0220, d_tracer,
> - tr, &tracing_mark_fops);
> + trace_create_file("trace_marker", 0220, d_tracer,
> + tr, &tracing_mark_fops);
>
> - tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");
> + tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");
>
> - trace_create_file("trace_marker_raw", 0220, d_tracer,
> - tr, &tracing_mark_raw_fops);
> + trace_create_file("trace_marker_raw", 0220, d_tracer,
> + tr, &tracing_mark_raw_fops);
>
> - trace_create_file("trace_clock", TRACE_MODE_WRITE, d_tracer, tr,
> + trace_create_file("buffer_percent", TRACE_MODE_WRITE, d_tracer,
> + tr, &buffer_percent_fops);
> +
> + trace_create_file("syscall_user_buf_size", TRACE_MODE_WRITE, d_tracer,
> + tr, &tracing_syscall_buf_fops);
> + }
> +
> + trace_create_file("trace_clock", writable_mode, d_tracer, tr,
> &trace_clock_fops);
>
> - trace_create_file("tracing_on", TRACE_MODE_WRITE, d_tracer,
> + trace_create_file("tracing_on", writable_mode, d_tracer,
> tr, &rb_simple_fops);
Hmm, should tracing_on exist in read only mode?
>
> trace_create_file("timestamp_mode", TRACE_MODE_READ, d_tracer, tr,
> @@ -10680,41 +10720,38 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
>
> tr->buffer_percent = 50;
>
> - trace_create_file("buffer_percent", TRACE_MODE_WRITE, d_tracer,
> - tr, &buffer_percent_fops);
> -
> - trace_create_file("buffer_subbuf_size_kb", TRACE_MODE_WRITE, d_tracer,
> + trace_create_file("buffer_subbuf_size_kb", writable_mode, d_tracer,
> tr, &buffer_subbuf_size_fops);
>
> - trace_create_file("syscall_user_buf_size", TRACE_MODE_WRITE, d_tracer,
> - tr, &tracing_syscall_buf_fops);
> -
> create_trace_options_dir(tr);
>
> #ifdef CONFIG_TRACER_MAX_TRACE
> - trace_create_maxlat_file(tr, d_tracer);
> + if (!readonly)
> + trace_create_maxlat_file(tr, d_tracer);
> #endif
>
> - if (ftrace_create_function_files(tr, d_tracer))
> + if (!readonly && ftrace_create_function_files(tr, d_tracer))
> MEM_FAIL(1, "Could not allocate function filter files");
>
> if (tr->range_addr_start) {
> trace_create_file("last_boot_info", TRACE_MODE_READ, d_tracer,
> tr, &last_boot_fops);
> #ifdef CONFIG_TRACER_SNAPSHOT
> - } else {
> + } else if (!readonly) {
> trace_create_file("snapshot", TRACE_MODE_WRITE, d_tracer,
> tr, &snapshot_fops);
> #endif
> }
>
> - trace_create_file("error_log", TRACE_MODE_WRITE, d_tracer,
> - tr, &tracing_err_log_fops);
> + if (!readonly)
> + trace_create_file("error_log", TRACE_MODE_WRITE, d_tracer,
> + tr, &tracing_err_log_fops);
Why not move this up into the "if (!readonly) {" block above?
>
> for_each_tracing_cpu(cpu)
> tracing_init_tracefs_percpu(tr, cpu);
>
> - ftrace_init_tracefs(tr, d_tracer);
> + if (!readonly)
> + ftrace_init_tracefs(tr, d_tracer);
Same here. Or just move the readonly block down to the end of the function.
> }
>
> #ifdef CONFIG_TRACEFS_AUTOMOUNT_DEPRECATED
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 69e7defba6c6..0adc644084bf 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -33,6 +33,7 @@
>
> #define TRACE_MODE_WRITE 0640
> #define TRACE_MODE_READ 0440
> +#define TRACE_MODE_WRITE_MASK (TRACE_MODE_WRITE & ~TRACE_MODE_READ)
>
> enum trace_type {
> __TRACE_FIRST_TYPE = 0,
> @@ -483,6 +484,12 @@ extern bool trace_clock_in_ns(struct trace_array *tr);
>
> extern unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr);
>
> +static inline bool trace_array_is_readonly(struct trace_array *tr)
> +{
> + /* backup instance is read only. */
> + return tr->flags & TRACE_ARRAY_FL_VMALLOC;
Hmm, I wonder if we should create a RDONLY flag for the trace_array?
-- Steve
> +}
> +
> /*
> * The global tracer (top) should be the first trace array added,
> * but we check the flag anyway.
> @@ -681,7 +688,6 @@ struct dentry *trace_create_file(const char *name,
> void *data,
> const struct file_operations *fops);
>
> -
> /**
> * tracer_tracing_is_on_cpu - show real state of ring buffer enabled on for a cpu
> * @tr : the trace array to know if ring buffer is enabled
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index dbe29b4c6a7a..2ca2541c8a58 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -61,7 +61,8 @@ trace_boot_set_instance_options(struct trace_array *tr, struct xbc_node *node)
> v = memparse(p, NULL);
> if (v < PAGE_SIZE)
> pr_err("Buffer size is too small: %s\n", p);
> - if (tracing_resize_ring_buffer(tr, v, RING_BUFFER_ALL_CPUS) < 0)
> + if (trace_array_is_readonly(tr) ||
> + tracing_resize_ring_buffer(tr, v, RING_BUFFER_ALL_CPUS) < 0)
> pr_err("Failed to resize trace buffer to %s\n", p);
> }
>
> @@ -597,7 +598,7 @@ trace_boot_enable_tracer(struct trace_array *tr, struct xbc_node *node)
>
> p = xbc_node_find_value(node, "tracer", NULL);
> if (p && *p != '\0') {
> - if (tracing_set_tracer(tr, p) < 0)
> + if (trace_array_is_readonly(tr) || tracing_set_tracer(tr, p) < 0)
> pr_err("Failed to set given tracer: %s\n", p);
> }
>
Powered by blists - more mailing lists