linux-kernel - Re: [RFC PATCH 1/1] trace: Move trace event enable from fs_initcall to early

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]

Date:	Thu, 16 Aug 2012 17:18:03 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Ezequiel Garcia <elezegarcia@...il.com>
Cc:	linux-kernel@...r.kernel.org, Pekka Enberg <penberg@...nel.org>,
	tim.bird@...sony.com, lizefan@...wei.com,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [RFC PATCH 1/1] trace: Move trace event enable from fs_initcall
 to early_initcall

On Wed, 2012-08-15 at 12:18 -0300, Ezequiel Garcia wrote:
> This patch splits trace event initialization in two stages:
>  * ftrace enable
>  * sysfs event entry creation
> 
> This allows to capture trace events from an earlier point
> by using 'trace_event' kernel parameter and is important
> to trace boot-up allocations.
> 
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@...il.com>
> ---
>  kernel/trace/trace_events.c |   71 ++++++++++++++++++++++++++++++++-----------
>  1 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 29111da..3055bc9 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1450,6 +1450,49 @@ static __init int setup_trace_event(char *str)
>  }
>  __setup("trace_event=", setup_trace_event);
>  
> +static __init int event_trace_enable(void)
> +{
> +	struct ftrace_event_call **iter, *call;
> +	char *buf = bootup_event_buf;
> +	char *token;
> +	int ret;
> +
> +	for_each_event(iter, __start_ftrace_events, __stop_ftrace_events) {
> +
> +		call = *iter;
> +
> +		/* The linker may leave blanks */

Actually, this shouldn't be true anymore. I know you copied the comment,
but with the new pointer settings, there should be no blanks. We
probably should turn this into a WARN_ON().


> +		if (!call->name)
> +			continue;
> +
> +		if (!call->class->raw_init)
> +			continue;
> +
> +		ret = call->class->raw_init(call);
> +		if (ret < 0) {
> +			if (ret != -ENOSYS)
> +				pr_warning("Could not initialize trace events/%s\n",
> +						call->name);
> +			continue;
> +		}
> +		list_add(&call->list, &ftrace_events);

Anyway, this code is duplicated now here and in
__trace_add_event_call(). A helper function should be created to store
this in both locations.

> +	}
> +
> +	while (true) {
> +		token = strsep(&buf, ",");
> +
> +		if (!token)
> +			break;
> +		if (!*token)
> +			continue;
> +
> +		ret = ftrace_set_clr_event(token, 1);
> +		if (ret)
> +			pr_warning("Failed to enable trace event: %s\n", token);
> +	}
> +	return 0;
> +}
> +
>  static __init int event_trace_init(void)
>  {
>  	struct ftrace_event_call **call;
> @@ -1457,8 +1500,6 @@ static __init int event_trace_init(void)
>  	struct dentry *entry;
>  	struct dentry *d_events;
>  	int ret;
> -	char *buf = bootup_event_buf;
> -	char *token;
>  
>  	d_tracer = tracing_init_dentry();
>  	if (!d_tracer)
> @@ -1497,24 +1538,17 @@ static __init int event_trace_init(void)
>  	if (trace_define_common_fields())
>  		pr_warning("tracing: Failed to allocate common fields");
>  
> +	/*
> +	 * Early initialization already enabled ftrace event.
> +	 * Now it's only necessary to create the event directory.
> +	 */
>  	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
> -		__trace_add_event_call(*call, NULL, &ftrace_event_id_fops,
> -				       &ftrace_enable_fops,
> -				       &ftrace_event_filter_fops,
> -				       &ftrace_event_format_fops);
> -	}
> -
> -	while (true) {
> -		token = strsep(&buf, ",");
> -
> -		if (!token)
> -			break;
> -		if (!*token)
> -			continue;
>  
> -		ret = ftrace_set_clr_event(token, 1);
> -		if (ret)
> -			pr_warning("Failed to enable trace event: %s\n", token);
> +		event_create_dir(*call, d_events,
> +				 &ftrace_event_id_fops,
> +				 &ftrace_enable_fops,
> +				 &ftrace_event_filter_fops,
> +				 &ftrace_event_format_fops);

This changes how events are currently. If we fail to create the
directory, we still keep the event. This can become confusing as events
and the directories will not match anymore. A failure should be checked
for, and if it happens, the event should be removed.

-- Steve

>  	}
>  
>  	ret = register_module_notifier(&trace_module_nb);
> @@ -1523,6 +1557,7 @@ static __init int event_trace_init(void)
>  
>  	return 0;
>  }
> +early_initcall(event_trace_enable);
>  fs_initcall(event_trace_init);
>  
>  #ifdef CONFIG_FTRACE_STARTUP_TEST


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/