Re: [PATCH 1/1] tracing: Fix memory leak when reading set_event file

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 19 Feb 2025 18:42:30 +0800
Adrian Huang <adrianhuang0701@gmail.com> wrote:


> The root cause is that s_next() returns NULL when nothing is found.
> This results in s_stop() attempting to free a NULL pointer because its
> parameter p is NULL.
> 
> Fix the issue by freeing the memory appropriately when s_next() fails
> to find anything.
> 
> Fixes: b355247df104 ("tracing: Cache ":mod:" events for modules not loaded yet")
> Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> ---
>  kernel/trace/trace_events.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 4cb275316e51..c76353ad0a4e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1591,6 +1591,7 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
>  		return iter;
>  #endif
>  
> +	kfree(iter);
>  	return NULL;
>  }
>  

This most definitely needs a comment, as it will look like a bug otherwise.
Please add:

	/*
	 * The iter is allocated in s_start() and passed via the 'v'
	 * parameter. To stop the iterator, NULL must be returned. But
	 * the return value is what the 'v' parameter in s_stop() receives
	 * and frees. Free iter here as it will no longer be used.
	 */
	kfree(iter);

I would also change the variable 'p' in s_stop() to 'v' to be consistent.

Thanks,

-- Steve