Re: [PATCH v5 2/2] tracing: Show last module text symbols in the stacktrace

[Steven Rostedt]

On Tue, 18 Mar 2025 22:39:21 +0900
"Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:


> +/**
> + * 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;
> +	struct trace_mod_entry *entry;
> +	long *module_delta;
> +	int idx = 0, nr_entries;
> +
> +	/* If we don't have last boot delta, return the address */
> +	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
> +		return addr;
> +
> +	tscratch = tr->scratch;
> +	/* if there is no tscrach, module_delta must be NULL. */
> +	module_delta = READ_ONCE(tr->module_delta);

What protects this from being freed after it is read?

> +	if (!module_delta || tscratch->entries[0].mod_addr > addr)
> +		return addr + tr->text_delta;
> +
> +	/* Note that entries must be sorted. */
> +	nr_entries = tscratch->nr_entries;
> +	if (nr_entries == 1 ||
> +	    tscratch->entries[nr_entries - 1].mod_addr < addr)
> +		idx = nr_entries - 1;
> +	else {
> +		entry = __inline_bsearch((void *)addr,
> +				tscratch->entries,
> +				nr_entries - 1,
> +				sizeof(tscratch->entries[0]),
> +				cmp_mod_entry);
> +		if (entry)
> +			idx = entry - tscratch->entries;
> +	}
> +
> +	return addr + module_delta[idx];
> +}
> +
>  #ifdef CONFIG_MODULES
>  static int save_mod(struct module *mod, void *data)
>  {
> @@ -6036,6 +6088,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;
> @@ -6070,6 +6123,8 @@ static void update_last_data(struct trace_array *tr)
>  		return;
>  
>  	tscratch = tr->scratch;
> +	module_delta = READ_ONCE(tr->module_delta);

Say if a reader read tr->module_delta before the NULL write.

> +	WRITE_ONCE(tr->module_delta, NULL);
>  
>  	/* Set the persistent ring buffer meta data to this address */
>  #ifdef CONFIG_RANDOMIZE_BASE
> @@ -6078,6 +6133,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);

Why is this safe?

I don't see any synchronization between setting NULL and freeing this,
like RCU would do.

-- Steve


>  }
>