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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 21 Mar 2025 12:52:03 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 18 Mar 2025 22:39:21 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> 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.

Ah, I thought it is OK that module_delta = NULL for kfree(), but
there could be UAF case?  update_last_data() is protected by trace_types_lock,
so update_last_data() itself is serialized. But trace_adjust_address() is
not. Hmm, yeah, it is not enough checking by TRACE_ARRAY_FL_LAST_BOOT flag.

OK, then what about this?

- free module_delta with rcu_free()
- protect trace_adjust_address() by rcu_read_lock()

Thank you,

> 
> -- Steve
> 
> 
> >  }
> > 


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