Re: PATCH] tracing: Revert commit 5ad289336148 ("tracing: Use a single array to represent tracer options files")

[Steven Rostedt <rostedt@goodmis.org>]

Oh good, this commit was never pushed to Linus, so I can just fold it
all together. Phew!

[ sorry for the noise ]

-- Steve


On Thu, 10 Sep 2026 21:40:08 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The commit 5ad289336148 ("tracing: Use a single array to represent tracer
> options files") simplified the array that stores the individual tracer
> options within the trace_array from being a multi array where each tracer
> had its own array for its options to a single array where all the tracers
> options were in a single array of the trace_array.
> 
> Testing has shown why it was done with multiple arrays in the first place.
> The single array use krealloc() to increase the size of the array when new
> tracers are added. A krealloc can change the location of the array if it
> needs more memory than the current location can provide. The files
> associated with the arrays passed in the address of these elements, and
> when the array changed, the elements became stale and no longer pointed to
> the element that represented the option.
> 
> Luckily, the reason this was done was also to check on open if the element
> still existed or if it was removed when an instance directory was removed.
> On opening a file with a stale element due to the array moving from a
> krealloc(), it would not find a corresponding trace_array and the open
> would return -ENODEV.
> 
> Revert the code back to multi-array where only the outer array gets
> resized and its elements point to the allocated arrays for each tracer.
> 
> Update the trace_array_trace_options_get() function to search each tracer
> array of the main array of the trace_array using a new helper function
> tr_option_match(). This preserves the logic to find an existing
> trace_array corresponding to the trace options descriptor and doesn't
> suffer from krealloc() moving the elements around.
> 
> Fixes: e8ea6a874574 ("tracing: Take trace_array reference when opening a tracer options file")
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c | 63 +++++++++++++++++++++++++++++---------------
>  kernel/trace/trace.h | 22 ++++++++++------
>  2 files changed, 56 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 7cc3fac70216..e4a490d3d08c 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7678,7 +7678,7 @@ static ssize_t
>  trace_options_read(struct file *filp, char __user *ubuf, size_t cnt,
>  			loff_t *ppos)
>  {
> -	struct trace_options *topt = filp->private_data;
> +	struct trace_option_dentry *topt = filp->private_data;
>  	char *buf;
>  
>  	if (topt->flags->val & topt->opt->bit)
> @@ -7693,7 +7693,7 @@ static ssize_t
>  trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  			 loff_t *ppos)
>  {
> -	struct trace_options *topt = filp->private_data;
> +	struct trace_option_dentry *topt = filp->private_data;
>  	unsigned long val;
>  	int ret;
>  
> @@ -7717,6 +7717,18 @@ trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	return cnt;
>  }
>  
> +static bool tr_option_match(struct trace_array *tr, void *topt)
> +{
> +	for (int i = 0; i < tr->nr_topts; i++) {
> +		struct trace_options *tr_topts = &tr->topts[i];
> +
> +		if (topt >= (void *)&tr_topts->topts[0] &&
> +		    topt < (void *)&tr_topts->topts[tr_topts->nr_topts])
> +			return true;
> +	}
> +	return false;
> +}
> +
>  /*
>   * The topt is the address of a trace_array->topts[] element that holds the
>   * the tracer options descriptor. But since the trace_array reference has not
> @@ -7742,8 +7754,7 @@ static int trace_array_tracer_options_get(void *topt)
>  
>  	guard(mutex)(&trace_types_lock);
>  	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> -		if (topt >= (void *)&tr->topts[0] &&
> -		    topt < (void *)&tr->topts[tr->nr_topts])
> +		if (tr_option_match(tr, topt))
>  			return __trace_array_get(tr);
>  	}
>  	return -ENODEV;
> @@ -7751,7 +7762,7 @@ static int trace_array_tracer_options_get(void *topt)
>  
>  static int tracing_open_options(struct inode *inode, struct file *filp)
>  {
> -	struct trace_options *topt = inode->i_private;
> +	struct trace_option_dentry *topt = inode->i_private;
>  	int ret;
>  
>  	ret = trace_array_tracer_options_get(topt);
> @@ -7764,7 +7775,7 @@ static int tracing_open_options(struct inode *inode, struct file *filp)
>  
>  static int tracing_release_options(struct inode *inode, struct file *file)
>  {
> -	struct trace_options *topt = file->private_data;
> +	struct trace_option_dentry *topt = file->private_data;
>  
>  	trace_array_put(topt->tr);
>  	return 0;
> @@ -7967,9 +7978,8 @@ static struct dentry *trace_options_init_dentry(struct trace_array *tr)
>  
>  static void
>  create_trace_option_file(struct trace_array *tr,
> -			 struct trace_options *topt,
> +			 struct trace_option_dentry *topt,
>  			 struct tracer_flags *flags,
> -			 struct tracer *tracer,
>  			 struct tracer_opt *opt)
>  {
>  	struct dentry *t_options;
> @@ -7981,7 +7991,6 @@ create_trace_option_file(struct trace_array *tr,
>  	topt->flags = flags;
>  	topt->opt = opt;
>  	topt->tr = tr;
> -	topt->tracer = tracer;
>  
>  	topt->entry = trace_create_file(opt->name, TRACE_MODE_WRITE,
>  					t_options, topt, &trace_options_fops);
> @@ -7991,9 +8000,10 @@ static int
>  create_trace_option_files(struct trace_array *tr, struct tracer *tracer,
>  			  struct tracer_flags *flags)
>  {
> -	struct trace_options *topts;
> +	struct trace_option_dentry *topts;
> +	struct trace_options *tr_topts;
>  	struct tracer_opt *opts;
> -	int i, cnt;
> +	int cnt;
>  
>  	if (!flags || !flags->opts)
>  		return 0;
> @@ -8003,22 +8013,30 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer,
>  	for (cnt = 0; opts[cnt].name; cnt++)
>  		;
>  
> -	topts = krealloc_array(tr->topts, tr->nr_topts + cnt, sizeof(*tr->topts),
> -				  GFP_KERNEL);
> +	topts = kzalloc_objs(*topts, cnt + 1);
>  	if (!topts)
> +		return 0;
> +
> +	tr_topts = krealloc_array(tr->topts, tr->nr_topts + 1, sizeof(*tr->topts),
> +				  GFP_KERNEL);
> +	if (!tr_topts) {
> +		kfree(topts);
>  		return -ENOMEM;
> +	}
>  
> -	tr->topts = topts;
> +	tr->topts = tr_topts;
> +	tr->topts[tr->nr_topts].tracer = tracer;
> +	tr->topts[tr->nr_topts].topts = topts;
> +	tr->topts[tr->nr_topts].nr_topts = cnt;
> +	tr->nr_topts++;
>  
> -	for (topts += tr->nr_topts, i = 0; i < cnt; topts++, i++) {
> -		create_trace_option_file(tr, topts, flags, tracer,
> -					 &opts[i]);
> -		MEM_FAIL(topts->entry == NULL,
> +	for (cnt = 0; opts[cnt].name; cnt++) {
> +		create_trace_option_file(tr, &topts[cnt], flags,
> +					 &opts[cnt]);
> +		MEM_FAIL(topts[cnt].entry == NULL,
>  			  "Failed to create trace option: %s",
> -			  opts[i].name);
> +			  opts[cnt].name);
>  	}
> -
> -	tr->nr_topts += cnt;
>  	return 0;
>  }
>  
> @@ -8880,6 +8898,9 @@ static int __remove_instance(struct trace_array *tr)
>  	if (tr->flags & TRACE_ARRAY_FL_VMALLOC)
>  		vfree((void *)tr->range_addr_start);
>  
> +	for (i = 0; i < tr->nr_topts; i++) {
> +		kfree(tr->topts[i].topts);
> +	}
>  	kfree(tr->topts);
>  
>  	free_cpumask_var(tr->pipe_cpumask);
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index fd0919a2c375..bd3c8f80300f 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -224,6 +224,12 @@ struct array_buffer {
>  
>  #define TRACE_FLAGS_MAX_SIZE		64
>  
> +struct trace_options {
> +	struct tracer			*tracer;
> +	struct trace_option_dentry	*topts;
> +	int				nr_topts;
> +};
> +
>  struct trace_pid_list *trace_pid_list_alloc(void);
>  void trace_pid_list_free(struct trace_pid_list *pid_list);
>  bool trace_pid_list_is_set(struct trace_pid_list *pid_list, unsigned int pid);
> @@ -320,14 +326,6 @@ struct trace_module_delta {
>  	long		delta[];
>  };
>  
> -struct trace_options {
> -	struct tracer_opt		*opt;
> -	struct tracer_flags		*flags;
> -	struct trace_array		*tr;
> -	struct tracer			*tracer;
> -	struct dentry			*entry;
> -};
> -
>  /*
>   * The trace array - an array of per-CPU trace arrays. This is the
>   * highest level data structure that individual tracers deal with.
> @@ -608,6 +606,14 @@ struct tracer_flags {
>  /* Makes more easy to define a tracer opt */
>  #define TRACER_OPT(s, b)	.name = #s, .bit = b
>  
> +
> +struct trace_option_dentry {
> +	struct tracer_opt		*opt;
> +	struct tracer_flags		*flags;
> +	struct trace_array		*tr;
> +	struct dentry			*entry;
> +};
> +
>  /**
>   * struct tracer - a specific tracer and its callbacks to interact with tracefs
>   * @name: the name chosen to select it on the available_tracers file