Re: [PATCH v3] tracing: Show last module text symbols in the stacktrace

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 10 Mar 2025 22:47:27 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the previous boot trace buffer can include module text address in
> the stacktrace. As same as the kernel text address, convert the module
> text address using the module address information.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v3:
>   - Move module_delta to trace_scratch data structure.
>   - Remove LRU based removed module information.
> ---
>  kernel/trace/trace.c        |   99 +++++++++++++++++++++++++++++++++++++++++--
>  kernel/trace/trace.h        |    2 +
>  kernel/trace/trace_output.c |    4 +-
>  3 files changed, 98 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index c3c79908766e..0c1aa1750077 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -49,6 +49,7 @@
>  #include <linux/fsnotify.h>
>  #include <linux/irq_work.h>
>  #include <linux/workqueue.h>
> +#include <linux/sort.h>
>  
>  #include <asm/setup.h> /* COMMAND_LINE_SIZE and kaslr_offset() */
>  
> @@ -5996,11 +5997,41 @@ struct trace_mod_entry {
>  struct trace_scratch {
>  	unsigned long		kaslr_addr;
>  	unsigned long		nr_entries;
> +	long			*module_delta;

Why are we adding this pointer into the persistent memory when it is
useless after a crash?

>  	struct trace_mod_entry	entries[];
>  };
>  
>  static DEFINE_MUTEX(scratch_mutex);
>  
> +/**
> + * trace_adjust_address() - Adjust prev boot address to current address.
> + * @tr: Persistent ring buffer's trace_array.
> + * @addr: Address in @tr which is adjusted.
> + */
> +unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr)
> +{
> +	struct trace_scratch *tscratch;
> +	long *module_delta;
> +	int i;
> +
> +	/* If we don't have last boot delta, return the address */
> +	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
> +		return addr;
> +
> +	tscratch = tr->scratch;
> +	module_delta = READ_ONCE(tscratch->module_delta);
> +	if (!tscratch || !tscratch->nr_entries || !module_delta ||

You shouldn't have a !tscratch test just after dereferencing it:

		tscratch->module_delta

Perhaps have it be:

	module_delta = tscratch ? READ_ONCE(tscratch->module_delta) : 0;
	if (!module_delta || !tscratch->nr_entries || tscratch->entries[0].mod_addr > addr)


> +	    tscratch->entries[0].mod_addr > addr)
> +		return addr + tr->text_delta;
> +
> +	/* Note that entries must be sorted. */
> +	for (i = 0; i < tscratch->nr_entries; i++)
> +		if (addr < tscratch->entries[i].mod_addr)
> +			break;

If we are bother sorting it, why not do a binary search here?

> +
> +	return addr + module_delta[i - 1];
> +}
> +
>  static int save_mod(struct module *mod, void *data)
>  {
>  	struct trace_array *tr = data;
> @@ -6029,6 +6060,7 @@ static int save_mod(struct module *mod, void *data)
>  static void update_last_data(struct trace_array *tr)
>  {
>  	struct trace_scratch *tscratch;
> +	long *module_delta;
>  
>  	if (!(tr->flags & TRACE_ARRAY_FL_BOOT))
>  		return;
> @@ -6063,6 +6095,8 @@ static void update_last_data(struct trace_array *tr)
>  		return;
>  
>  	tscratch = tr->scratch;
> +	module_delta = READ_ONCE(tscratch->module_delta);
> +	WRITE_ONCE(tscratch->module_delta, NULL);
>  
>  	/* Set the persistent ring buffer meta data to this address */
>  #ifdef CONFIG_RANDOMIZE_BASE
> @@ -6071,6 +6105,8 @@ static void update_last_data(struct trace_array *tr)
>  	tscratch->kaslr_addr = 0;
>  #endif
>  	tr->flags &= ~TRACE_ARRAY_FL_LAST_BOOT;
> +
> +	kfree(module_delta);
>  }
>  
>  /**
> @@ -9342,10 +9378,43 @@ static struct dentry *trace_instance_dir;
>  static void
>  init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer);
>  
> +static int make_mod_delta(struct module *mod, void *data)
> +{
> +	struct trace_scratch *tscratch;
> +	struct trace_mod_entry *entry;
> +	struct trace_array *tr = data;
> +	long *module_delta;
> +	int i;
> +
> +	tscratch = tr->scratch;
> +	module_delta = READ_ONCE(tscratch->module_delta);
> +	for (i = 0; i < tscratch->nr_entries; i++) {
> +		entry = &tscratch->entries[i];
> +		if (!strcmp(mod->name, entry->mod_name)) {
> +			if (mod->state == MODULE_STATE_GOING)
> +				module_delta[i] = 0;
> +			else
> +				module_delta[i] = (unsigned long)mod->mem[MOD_TEXT].base
> +						 - entry->mod_addr;
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int mod_addr_comp(const void *a, const void *b, const void *data)
> +{
> +	const struct trace_mod_entry *e1 = a;
> +	const struct trace_mod_entry *e2 = b;
> +
> +	return e1->mod_addr > e2->mod_addr ? 1 : -1;
> +}
> +
>  static void setup_trace_scratch(struct trace_array *tr,
>  				struct trace_scratch *tscratch, unsigned int size)
>  {
>  	struct trace_mod_entry *entry;
> +	int i, nr_entries;
>  
>  	if (!tscratch)
>  		return;
> @@ -9362,7 +9431,7 @@ static void setup_trace_scratch(struct trace_array *tr,
>  		goto reset;
>  
>  	/* Check if each module name is a valid string */
> -	for (int i = 0; i < tscratch->nr_entries; i++) {
> +	for (i = 0; i < tscratch->nr_entries; i++) {
>  		int n;
>  
>  		entry = &tscratch->entries[i];
> @@ -9376,6 +9445,21 @@ static void setup_trace_scratch(struct trace_array *tr,
>  		if (n == MODULE_NAME_LEN)
>  			goto reset;
>  	}
> +
> +	nr_entries = i;
> +	tscratch->module_delta = kcalloc(nr_entries, sizeof(long), GFP_KERNEL);
> +	if (!tscratch->module_delta) {
> +		pr_info("module_delta allocation failed. Not able to decode module address.");
> +		goto reset;
> +	}
> +
> +	/* Sort the entries so that we can find appropriate module from address. */
> +	sort_r(tscratch->entries, nr_entries, sizeof(struct trace_mod_entry),
> +	       mod_addr_comp, NULL, NULL);
> +
> +	/* Scan modules to make text delta for modules. */
> +	module_for_each_mod(make_mod_delta, tr);
> +
>  	return;
>   reset:
>  	/* Invalid trace modules */
> @@ -10101,19 +10185,23 @@ static bool trace_array_active(struct trace_array *tr)
>  	return trace_events_enabled(tr, NULL) > 1;
>  }
>  
> -static void trace_module_record(struct module *mod)
> +static void trace_module_record(struct module *mod, bool remove)
>  {
>  	struct trace_array *tr;
> +	unsigned long flags;
>  
>  	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> +		flags = tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT);
>  		/* Update any persistent trace array that has already been started */
> -		if ((tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT)) ==
> -		    TRACE_ARRAY_FL_BOOT) {
> +		if (flags == TRACE_ARRAY_FL_BOOT && !remove) {

Can you rename the parameter from "remove" to "add" so we don't have a
double negative.

		if (flags == TRACE_ARRAY_FL_BOOT && add) {


>  			/* Only update if the trace array is active */
>  			if (trace_array_active(tr)) {
>  				guard(mutex)(&scratch_mutex);
>  				save_mod(mod, tr);
>  			}
> +		} else if (flags & TRACE_ARRAY_FL_LAST_BOOT) {
> +			/* Update delta if the module loaded in previous boot */
> +			make_mod_delta(mod, tr);
>  		}
>  	}
>  }
> @@ -10126,10 +10214,11 @@ static int trace_module_notify(struct notifier_block *self,
>  	switch (val) {
>  	case MODULE_STATE_COMING:
>  		trace_module_add_evals(mod);
> -		trace_module_record(mod);
> +		trace_module_record(mod, false);

		trace_module_record(mod, true);

>  		break;
>  	case MODULE_STATE_GOING:
>  		trace_module_remove_evals(mod);
> +		trace_module_record(mod, true);

		trace_module_record(mod, false);

>  		break;
>  	}
>  
>

-- Steve