linux-kernel - Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

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:   Wed, 22 Nov 2017 14:50:02 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     "Vladislav Valtchev (VMware)" <vladislav.valtchev@...il.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack
 tracer is ON

On Wed, 22 Nov 2017 20:02:02 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@...il.com> wrote:

> trace-cmd stat is a handy way for users to see what tracing is currently going
> on, but currently is does not say anything about the stack tracing. This simple
> patch makes the command to show a message when the stack tracer is ON.

I applied the first two. Small comments about this one.

> 
> Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@...il.com>
> ---
>  trace-cmd.h   |  3 +++
>  trace-stack.c | 10 ++++++----
>  trace-stat.c  | 11 +++++++----
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/trace-cmd.h b/trace-cmd.h
> index 485907f..62c5ea7 100644
> --- a/trace-cmd.h
> +++ b/trace-cmd.h
> @@ -351,6 +351,9 @@ struct hook_list {
>  struct hook_list *tracecmd_create_event_hook(const char *arg);
>  void tracecmd_free_hooks(struct hook_list *hooks);
>  
> +/* Stack tracer public functions */
> +int is_stack_tracer_enabled(void);

As this is now in the trace-cmd.h header, please rename it to:

 tracecmd_is_stack_tracer_enabled()

> +
>  /* --- Hack! --- */
>  int tracecmd_blk_hack(struct tracecmd_input *handle);
>  
> diff --git a/trace-stack.c b/trace-stack.c
> index aa79ae3..0bd43a8 100644
> --- a/trace-stack.c
> +++ b/trace-stack.c
> @@ -49,7 +49,7 @@ static void test_available(void)
>  		die("stack tracer not configured on running kernel");
>  }
>  
> -static char read_proc(void)
> +int is_stack_tracer_enabled(void)
>  {
>  	char buf[1];
>  	int fd;
> @@ -62,8 +62,10 @@ static char read_proc(void)
>  	close(fd);
>  	if (n != 1)
>  		die("error reading %s", PROC_FILE);
> +	if (buf[0] != '0' && buf[0] != '1')
> +		die("Invalid value '%c' in %s", buf[0], PROC_FILE);

Why kill it here? We are reading the proc file system. What happens if
a new kernel does update this. We just broke this tool, and we don't
break user space with kernel updates. But user space should also be
robust for updates like this.

Actually, what I suggest is to keep the static read_proc function, and
simply add:

bool tracecmd_is_stack_tracer_enabled(void)
{
	char buf;

	buf = read_proc();
	return buf != '0';
}

Much easier change. And handles cases where the proc file is 2 or more.

-- Steve


>  
> -	return buf[0];
> +	return buf[0] == '1';
>  }
>  
>  static void start_stop_trace(char val)
> @@ -72,7 +74,7 @@ static void start_stop_trace(char val)
>  	int fd;
>  	int n;
>  
> -	buf[0] = read_proc();
> +	buf[0] = '0' + is_stack_tracer_enabled();
>  	if (buf[0] == val)
>  		return;
>  
> @@ -124,7 +126,7 @@ static void read_trace(void)
>  	size_t n;
>  	int r;
>  
> -	if (read_proc() == '1')
> +	if (is_stack_tracer_enabled())
>  		printf("(stack tracer running)\n");
>  	else
>  		printf("(stack tracer not running)\n");
> diff --git a/trace-stat.c b/trace-stat.c
> index fd16354..3094d25 100644
> --- a/trace-stat.c
> +++ b/trace-stat.c
> @@ -617,7 +617,7 @@ static void report_graph_funcs(struct buffer_instance *instance)
>  		die("malloc");
>  
>  	list_functions(path, "Function Graph Filter");
> -	
> +
>  	tracecmd_put_tracing_file(path);
>  
>  	path = get_instance_file(instance, "set_graph_notrace");
> @@ -625,7 +625,7 @@ static void report_graph_funcs(struct buffer_instance *instance)
>  		die("malloc");
>  
>  	list_functions(path, "Function Graph No Trace");
> -	
> +
>  	tracecmd_put_tracing_file(path);
>  }
>  
> @@ -638,7 +638,7 @@ static void report_ftrace_filters(struct buffer_instance *instance)
>  		die("malloc");
>  
>  	list_functions(path, "Function Filter");
> -	
> +
>  	tracecmd_put_tracing_file(path);
>  
>  	path = get_instance_file(instance, "set_ftrace_notrace");
> @@ -646,7 +646,7 @@ static void report_ftrace_filters(struct buffer_instance *instance)
>  		die("malloc");
>  
>  	list_functions(path, "Function No Trace");
> -	
> +
>  	tracecmd_put_tracing_file(path);
>  }
>  
> @@ -928,5 +928,8 @@ void trace_stat (int argc, char **argv)
>  		stat_instance(instance);
>  	}
>  
> +	if (is_stack_tracer_enabled())
> +		printf("Stack tracing is enabled\n\n");
> +
>  	exit(0);
>  }