| Message ID | 20250205225103.930895467@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ring-buffer/tracing: Save module information in persistent memory | expand |
On Wed, 05 Feb 2025 17:50:37 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > For trace instances that are mapped to persistent memory, have them use > the scratch area to save the currently loaded modules. This will allow > where the modules have been loaded on the next boot so that their > addresses can be deciphered by using where they were loaded previously. > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/trace.c | 99 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 87 insertions(+), 12 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index cb9f8e6878a0..a8e5f7ac2193 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -5994,14 +5994,60 @@ ssize_t tracing_resize_ring_buffer(struct trace_array *tr, > return ret; > } > > +struct trace_mod_entry { > + unsigned long mod_addr; > + char mod_name[MODULE_NAME_LEN]; > +}; > + > struct trace_scratch { > unsigned long kaslr_addr; > + unsigned long nr_entries; > + struct trace_mod_entry entries[]; > }; > > +static int save_mod(struct module *mod, void *data) > +{ > + struct trace_array *tr = data; > + struct trace_scratch *tscratch; > + struct trace_mod_entry *entry; > + unsigned int size; > + > + tscratch = tr->scratch; > + if (!tscratch) > + return -1; > + size = tr->scratch_size; > + > + if (struct_size(tscratch, entries, tscratch->nr_entries + 1) > size) > + return -1; > + > + entry = &tscratch->entries[tscratch->nr_entries]; > + > + tscratch->nr_entries++; > + > + entry->mod_addr = (unsigned long)mod->mem[MOD_TEXT].base; > + strscpy(entry->mod_name, mod->name); > + > + return 0; > +} > + > static void update_last_data(struct trace_array *tr) > { > struct trace_scratch *tscratch; > > + if (!(tr->flags & TRACE_ARRAY_FL_BOOT)) > + return; > + > + /* Reset the module list and reload them */ > + if (tr->scratch) { > + struct trace_scratch *tscratch = tr->scratch; > + > + memset(tscratch->entries, 0, > + flex_array_size(tscratch, entries, tscratch->nr_entries)); > + tscratch->nr_entries = 0; > + > + module_for_each_mod(save_mod, tr); > + } This maybe too frequently scan the module because the update_last_data() is called when; - change tracer (this maybe OK) - update "set_event" - write 1 to "enable" under events - change pid filter - etc. Once it is scanned, it should not scan again, but update by module callback, because usually events are enabled individually (thus write 1 to "event" can happen multiple time online). I think we can move this after TRACE_ARRAY_FL_LAST_BOOT check, if TRACE_ARRAY_FL_LAST_BOOT flag is set, that flag should be cleared with updating the tscratch, and the flag is not set, we can skip updating the scratch. Thank you, > + > if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT)) > return; > > @@ -9226,6 +9272,46 @@ static struct dentry *trace_instance_dir; > static void > init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer); > > +static void setup_trace_scratch(struct trace_array *tr, void *scratch, unsigned int size) > +{ > + struct trace_scratch *tscratch = scratch; > + struct trace_mod_entry *entry; > + > + if (!scratch) > + return; > + > + tr->scratch = scratch; > + tr->scratch_size = size; > + > +#ifdef CONFIG_RANDOMIZE_BASE > + if (tscratch->kaslr_addr) > + tr->text_delta = kaslr_offset() - tscratch->kaslr_addr; > +#endif > + > + if (struct_size(tscratch, entries, tscratch->nr_entries) > size) > + goto reset; > + > + /* Check if each module name is a valid string */ > + for (int i = 0; i < tscratch->nr_entries; i++) { > + int n; > + > + entry = &tscratch->entries[i]; > + > + for (n = 0; n < MODULE_NAME_LEN; n++) { > + if (entry->mod_name[n] == '\0') > + break; > + if (!isprint(entry->mod_name[n])) > + goto reset; > + } > + if (n == MODULE_NAME_LEN) > + goto reset; > + } > + return; > + reset: > + /* Invalid trace modules */ > + memset(scratch, 0, size); > +} > + > static int > allocate_trace_buffer(struct trace_array *tr, struct array_buffer *buf, int size) > { > @@ -9243,19 +9329,8 @@ allocate_trace_buffer(struct trace_array *tr, struct array_buffer *buf, int size > tr->range_addr_size); > > scratch = ring_buffer_meta_scratch(buf->buffer, &scratch_size); > - if (scratch) { > - tr->scratch = scratch; > - tr->scratch_size = scratch_size; > + setup_trace_scratch(tr, scratch, scratch_size); > > -#ifdef CONFIG_RANDOMIZE_BASE > - { > - struct trace_scratch *tscratch = tr->scratch; > - > - if (tscratch->kaslr_addr) > - tr->text_delta = kaslr_offset() - tscratch->kaslr_addr; > - } > -#endif > - } > /* > * This is basically the same as a mapped buffer, > * with the same restrictions. > -- > 2.45.2 > >
On Thu, 6 Feb 2025 17:26:42 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > This maybe too frequently scan the module because the update_last_data() is > called when; > - change tracer (this maybe OK) > - update "set_event" > - write 1 to "enable" under events > - change pid filter > - etc. > > Once it is scanned, it should not scan again, but update by module > callback, because usually events are enabled individually (thus > write 1 to "event" can happen multiple time online). > > I think we can move this after TRACE_ARRAY_FL_LAST_BOOT check, > if TRACE_ARRAY_FL_LAST_BOOT flag is set, that flag should be cleared > with updating the tscratch, and the flag is not set, we can skip updating > the scratch. No, because we could enable it, disable it, and then the user could be adding and removing modules all day long, and we only add and do not remove (I do have a patch that does remove them). But we can do the same check as patch 8 does, and only do this if the tracer wasn't already active. -- Steve
On Thu, 6 Feb 2025 10:29:49 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 6 Feb 2025 17:26:42 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > This maybe too frequently scan the module because the update_last_data() is > > called when; > > - change tracer (this maybe OK) > > - update "set_event" > > - write 1 to "enable" under events > > - change pid filter > > - etc. > > > > Once it is scanned, it should not scan again, but update by module > > callback, because usually events are enabled individually (thus > > write 1 to "event" can happen multiple time online). > > > > I think we can move this after TRACE_ARRAY_FL_LAST_BOOT check, > > if TRACE_ARRAY_FL_LAST_BOOT flag is set, that flag should be cleared > > with updating the tscratch, and the flag is not set, we can skip updating > > the scratch. > > No, because we could enable it, disable it, and then the user could be > adding and removing modules all day long, and we only add and do not remove > (I do have a patch that does remove them). I think TRACE_ARRAY_FL_LAST_BOOT is something like one-way flag, it is set if the buffer is persistent, but cleared when it is reused (start writing) and never be back. Isn't it? > > But we can do the same check as patch 8 does, and only do this if the > tracer wasn't already active. Yeah, so I think this change needs patch 8 change. Thank you, > > -- Steve
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index cb9f8e6878a0..a8e5f7ac2193 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -5994,14 +5994,60 @@ ssize_t tracing_resize_ring_buffer(struct trace_array *tr, return ret; } +struct trace_mod_entry { + unsigned long mod_addr; + char mod_name[MODULE_NAME_LEN]; +}; + struct trace_scratch { unsigned long kaslr_addr; + unsigned long nr_entries; + struct trace_mod_entry entries[]; }; +static int save_mod(struct module *mod, void *data) +{ + struct trace_array *tr = data; + struct trace_scratch *tscratch; + struct trace_mod_entry *entry; + unsigned int size; + + tscratch = tr->scratch; + if (!tscratch) + return -1; + size = tr->scratch_size; + + if (struct_size(tscratch, entries, tscratch->nr_entries + 1) > size) + return -1; + + entry = &tscratch->entries[tscratch->nr_entries]; + + tscratch->nr_entries++; + + entry->mod_addr = (unsigned long)mod->mem[MOD_TEXT].base; + strscpy(entry->mod_name, mod->name); + + return 0; +} + static void update_last_data(struct trace_array *tr) { struct trace_scratch *tscratch; + if (!(tr->flags & TRACE_ARRAY_FL_BOOT)) + return; + + /* Reset the module list and reload them */ + if (tr->scratch) { + struct trace_scratch *tscratch = tr->scratch; + + memset(tscratch->entries, 0, + flex_array_size(tscratch, entries, tscratch->nr_entries)); + tscratch->nr_entries = 0; + + module_for_each_mod(save_mod, tr); + } + if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT)) return; @@ -9226,6 +9272,46 @@ static struct dentry *trace_instance_dir; static void init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer); +static void setup_trace_scratch(struct trace_array *tr, void *scratch, unsigned int size) +{ + struct trace_scratch *tscratch = scratch; + struct trace_mod_entry *entry; + + if (!scratch) + return; + + tr->scratch = scratch; + tr->scratch_size = size; + +#ifdef CONFIG_RANDOMIZE_BASE + if (tscratch->kaslr_addr) + tr->text_delta = kaslr_offset() - tscratch->kaslr_addr; +#endif + + if (struct_size(tscratch, entries, tscratch->nr_entries) > size) + goto reset; + + /* Check if each module name is a valid string */ + for (int i = 0; i < tscratch->nr_entries; i++) { + int n; + + entry = &tscratch->entries[i]; + + for (n = 0; n < MODULE_NAME_LEN; n++) { + if (entry->mod_name[n] == '\0') + break; + if (!isprint(entry->mod_name[n])) + goto reset; + } + if (n == MODULE_NAME_LEN) + goto reset; + } + return; + reset: + /* Invalid trace modules */ + memset(scratch, 0, size); +} + static int allocate_trace_buffer(struct trace_array *tr, struct array_buffer *buf, int size) { @@ -9243,19 +9329,8 @@ allocate_trace_buffer(struct trace_array *tr, struct array_buffer *buf, int size tr->range_addr_size); scratch = ring_buffer_meta_scratch(buf->buffer, &scratch_size); - if (scratch) { - tr->scratch = scratch; - tr->scratch_size = scratch_size; + setup_trace_scratch(tr, scratch, scratch_size); -#ifdef CONFIG_RANDOMIZE_BASE - { - struct trace_scratch *tscratch = tr->scratch; - - if (tscratch->kaslr_addr) - tr->text_delta = kaslr_offset() - tscratch->kaslr_addr; - } -#endif - } /* * This is basically the same as a mapped buffer, * with the same restrictions.