Re: [PATCH v2 1/2] tracing: Override the same name trace_instance= by latter one

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

On Fri, 02 Oct 2026 09:14:31 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] tracing: heap out-of-bounds write in
> parse_boot_instance_info due to unescaped tabs
> - [Medium] tracing: duplicate trace array creation via implicit
> backup lookup causes tracefs init failure --
> 
> commit e7d9ac9646a2e104af11665c8c61543d20737c13
> Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
>     tracing: Override the same name trace_instance= by latter one
>     
>     Override the same name boot instance by the latter parameter.
>     This allows user to override the trace instance in bootconfig by
> the cmdline from bootloader.
> 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index e4a490d3d08c8..4590d38969450 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -308,6 +309,7 @@ static int __init boot_instance(char *str)
> >  
> >  	ret = snprintf(slot, left, "%s\t", str);
> >  	boot_instance_index += ret;
> > +	nr_boot_instances++;
> >  
> >  	return 1;
> >  }  
> 
> [ ... ]
> 
> > +__init static void parse_boot_instance_info(struct boot_instance
> > *boot_instances) {  
> 
> [ ... ]
> 
> > +	while ((events = strsep(&str, "\t"))) {
> > +		if (!*events)
> > +			continue;  
> 
> [ ... ]
> 
> > +		for (i = 0; i < idx; i++) {
> > +			/* Override by the last defined instance */
> > +			if (!strcmp(name, boot_instances[i].name))
> > +				break;
> >  		}
> >  
> > +		boot_instances[i].name = name;
> > +		boot_instances[i].flag_delim = flag_delim;
> > +		boot_instances[i].addr_delim = addr_delim;
> > +		boot_instances[i].backup = backup;
> > +		boot_instances[i].events = events;  
> 
> [Severity: High]
> If a user provides a quoted parameter containing a literal tab (for
> example, trace_instance="foo\tbar"), won't strsep(&str, "\t") split
> it into more tokens than what was originally counted in
> nr_boot_instances?
> 
> This loop appears to write beyond the bounds of the boot_instances
> array when there are embedded tabs, which could lead to memory
> corruption and a kernel panic during early boot.

Ah, OK. Such usage is not recommended but it can happen and
causes array index overrun.

> 
> > +		if (i == idx)
> > +			idx++;
> > +	}
> > +
> > +	nr_boot_instances = idx;
> > +}
> > +
> > +__init static void enable_instances(void)
> > +{
> > +	struct boot_instance *boot_instances __free(kfree) = NULL;
> > +	struct boot_instance *bi;
> > +	struct trace_array *tr;
> > +	bool memmap_area = false;
> > +	char *tok;
> > +
> > +	if (!boot_instance_index)
> > +		return;
> > +
> > +	/* Parse the instance name to check the overlap */
> > +	boot_instances = kmalloc_array(nr_boot_instances,
> > +				       sizeof(*boot_instances),
> > GFP_KERNEL);
> > +	if (!boot_instances)
> > +		return;
> > +
> > +	parse_boot_instance_info(boot_instances);
> > +
> > +	for (int i = 0; i < nr_boot_instances; i++) {
> > +		char *rname __free(kfree) = NULL;  
> 
> [ ... ]
> 
> > -		if (trace_array_find(name)) {
> > -			pr_warn("Tracing: Instance %s already
> > exists\n", name);
> > -			continue;
> > -		}
> > -
> > -		if (backup) {
> > -			if (backup_instance_area(backup, &addr,
> > &size) < 0)
> > +		if (bi->backup) {
> > +			if (backup_instance_area(bi->backup,
> > &addr, &size) < 0) continue;
> >  		}  
> 
> [ ... ]
> 
> > -		tr = trace_array_create_systems(name, NULL, addr,
> > size);
> > +		tr = trace_array_create_systems(bi->name, NULL,
> > addr, size);  
> 
> [Severity: Medium]
> With the trace_array_find() check removed, does this unconditionally
> create a new trace array, even if one implicitly exists?

parse_boot_instance_info() already checks the overlaps.

> 
> If a user specifies a trace_instance with a backup pointing to
> another instance defined later (e.g., trace_instance=foo=bar
> trace_instance=bar), backup_instance_area() will implicitly create
> "bar" via trace_array_get_by_name().

Ah, that's a good point! we should check it in
parse_boot_instance_info().

> 
> When the loop processes "bar" explicitly later on, it seems this code
> will call trace_array_create_systems("bar", ...) a second time,
> duplicating the trace array in ftrace_trace_arrays. This duplicate
> could cause tracefs_create_dir() to fail and abort initialization of
> subsequent trace instances.
> 

Thank you,

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