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

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

On Thu, 20 Feb 2025 11:15:28 +0800
Adrian Huang <adrianhuang0701@gmail.com> wrote:

> From: Adrian Huang <ahuang12@lenovo.com>
> 
> kmemleak reports the following memory leak after reading set_event file:
> 
>   # cat /sys/kernel/tracing/set_event
> 
>   # cat /sys/kernel/debug/kmemleak
>   unreferenced object 0xff110001234449e0 (size 16):
>   comm "cat", pid 13645, jiffies 4294981880
>   hex dump (first 16 bytes):
>     01 00 00 00 00 00 00 00 a8 71 e7 84 ff ff ff ff  .........q......
>   backtrace (crc c43abbc):
>     __kmalloc_cache_noprof+0x3ca/0x4b0
>     s_start+0x72/0x2d0
>     seq_read_iter+0x265/0x1080
>     seq_read+0x2c9/0x420
>     vfs_read+0x166/0xc30
>     ksys_read+0xf4/0x1d0
>     do_syscall_64+0x79/0x150
>     entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> The issue can be reproduced regardless of whether set_event is empty or
> not. Here is an example about the valid content of set_event.
> 
>   # cat /sys/kernel/tracing/set_event
>   sched:sched_process_fork
>   sched:sched_switch
>   sched:sched_wakeup
>   *:*:mod:trace_events_sample
> 
> 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 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>

Looks good to me.

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

Thank you,

> ---
> Changelog v2:
>  - Per Steven's suggestion: Add a comment to describe why to free memory
>    in s_next().
>  - Per Steven's suggestion: Change the variable 'p' to 'v' in s_stop()
> 
> ---
>  kernel/trace/trace_events.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 4cb275316e51..513de9ceb80e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1591,6 +1591,13 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
>  		return iter;
>  #endif
>  
> +	/*
> +	 * 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);
>  	return NULL;
>  }
>  
> @@ -1667,9 +1674,9 @@ static int s_show(struct seq_file *m, void *v)
>  }
>  #endif
>  
> -static void s_stop(struct seq_file *m, void *p)
> +static void s_stop(struct seq_file *m, void *v)
>  {
> -	kfree(p);
> +	kfree(v);
>  	t_stop(m, NULL);
>  }
>  
> -- 
> 2.34.1
> 


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