linux-kernel - Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250211104122.8ce39b7d4be222318d6c010b@kernel.org>
Date: Tue, 11 Feb 2025 10:41:22 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, Mark
 Rutland <mark.rutland@....com>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Mike Rapoport <rppt@...nel.org>
Subject: Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

On Mon, 10 Feb 2025 19:13:50 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Tue, 11 Feb 2025 00:44:17 +0900
> "Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> 
> > @@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
> >  	int i;
> >  
> >  	/* Reference counter for a newly created trace array = 1. */
> > -	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
> > +	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
> > +		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);
> 
> I don't think we need the print. The function will error with -EBUSY
> letting the user know that it is busy. This is a very common error, where I
> found that I had a program that didn't completely die, and still had a file
> descriptor open on an instance, and when I go to remove it, I get:
> 
>   # cd /sys/kernel/tracing/
>   # mkdir instances/foo
>   # bash 5< instances/foo/events/enable
>   # rmdir instances/foo
>   rmdir: failed to remove 'instances/foo': Device or resource busy
>   # exit
>   # rmdir instances/foo
>   #
> 
> If you only have it for debugging that is fine, but we should remove it
> before we accept it.

Oops, that is my debug code. I missed to remove that! Sorry for confusion.

Thank you!

> 
> -- Steve
> 
> >  		return -EBUSY;
> > +	}
> >  
> >  	list_del(&tr->list);
> >  
> > @@ -9881,6 +9887,11 @@ static int __remove_instance(struct trace_array *tr)
> >  	free_trace_buffers(tr);
> >  	clear_tracing_err_log(tr);
> >  
> > +	if (tr->range_name) {
> > +		reserved_mem_release_by_name(tr->range_name);
> > +		kfree(tr->range_name);
> > +	}
> > +
> >  	for (i = 0; i < tr->nr_topts; i++) {
> >  		kfree(tr->topts[i].topts);
> >  	}
> > @@ -10740,6 +10751,7 @@ __init static void enable_instances(void)
> >  		bool traceoff = false;
> >  		char *flag_delim;
> >  		char *addr_delim;
> > +		char *rname __free(kfree) = NULL;
> >  
> >  		tok = strsep(&curr_str, ",");
> >  
> > @@ -10796,6 +10808,7 @@ __init static void enable_instances(void)
> >  				pr_warn("Failed to map boot instance %s to %s\n", name, tok);
> >  				continue;
> >  			}
> > +			rname = kstrdup(tok, GFP_KERNEL);
> >  		}
> >  
> >  		if (start) {
> > @@ -10832,7 +10845,7 @@ __init static void enable_instances(void)
> >  		 */
> >  		if (start) {
> >  			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
> > -			tr->ref++;
> > +			tr->range_name = no_free_ptr(rname);
> >  		}
> >  
> >  		while ((tok = strsep(&curr_str, ","))) {
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 47c0742fe9ec..ae8f7fac6592 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -348,6 +348,7 @@ struct trace_array {
> >  	unsigned int		mapped;
> >  	unsigned long		range_addr_start;
> >  	unsigned long		range_addr_size;
> > +	char			*range_name;
> >  	long			text_delta;
> >  	int			nr_modules;
> >  	long			*module_delta;
> 


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