Re: [PATCH RFC 3/7] tracing: Stop modifying the input buffer in ftrace_set_clr_event()

On Thu, Aug 13, 2026 at 10:38:21AM -0400, Steven Rostedt wrote:
> On Thu, 13 Aug 2026 14:15:00 +0000
> [email protected] wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Invalid free (causing kernel panic) and memory leak due to `strsep()` modifying a pointer managed by `__free(kfree)`.
> > --
> > 
> > --- Patch [3]: [PATCH RFC 3/7] tracing: Stop modifying the input buffer in ftrace_set_clr_event() ---
> > commit c968a27c39638474537de841b7e2191407463142
> > Author: Thomas Weißschuh <[email protected]>
> > 
> > tracing: Stop modifying the input buffer in ftrace_set_clr_event()
> > 
> > This commit changes ftrace_set_clr_event() to copy its input buffer using
> > kstrdup() and manage it with __free(kfree). This removes the need to
> > restore the original buffer modifications before returning.
> > 
> > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > > index 6aa32c4922807..c75a7f5b55def 100644
> > > --- a/kernel/trace/trace_events.c
> > > +++ b/kernel/trace/trace_events.c
> > > @@ -1411,14 +1411,17 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
> > >  	return ret;
> > >  }
> > >  
> > > -int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> > > +int ftrace_set_clr_event(struct trace_array *tr, const char *_buf, int set)
> > >  {
> > >  	char *event = NULL, *sub = NULL, *match, *mod;
> > >  
> > >  	if (!tr)
> > >  		return -ENOENT;
> > >  
> > > +	char *buf __free(kfree) = kstrdup(_buf, GFP_KERNEL);
> > > +	if (!buf)
> > > +		return -ENOMEM;
> > > +  
> > 
> > [Severity: High]
> > Will this lead to an invalid free or memory leak when buf is later modified? 
> > 
> > Further down in ftrace_set_clr_event(), buf is passed to strsep(), which
> > modifies the pointer in place:
> > 
> >     match = strsep(&buf, ":");
> > 
> > If strsep() advances the pointer to the next token, the cleanup function
> > will call kfree() on an interior pointer, causing a slab corruption bug or
> > kernel panic upon scope exit. If buf is advanced to NULL (when no colon is
> > found), it will result in a memory leak of the duplicated string since
> > kfree(NULL) is a no-op.
> > 
> > Could a separate pointer be used to track the allocation for cleanup,
> > leaving buf free to be safely advanced by strsep()?
> 
> Yes, this is a legitimate bug.
> 
> I guess you need to have:
> 
> int ftrace_set_clr_event(struct trace_array *tr, const char *buffer, int set)
> {
> 	char *event = NULL, *sub = NULL, *match, *mod;
> 	char *buf;
> 	int ret;
> 
> 	if (!tr)
> 		return -ENOENT;
> 
> 	char *_buf __free(kfree) = kstrdup(_buf, GFP_KERNEL);
> 	if (!_buf)
> 		return -ENOMEM;
> 
> 	/* Need to keep _buf pointing to the start of the buffer */
> 	buf = _buf;

Agreed.

I'll deal with the Sashiko fallout if you are fine with the general idea.


Thomas