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:	Tue, 06 Oct 2009 21:06:12 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Tom Zanussi <tzanussi@...il.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, fweisbec@...il.com,
	lizf@...fujitsu.com, hch@...radead.org
Subject: Re: [RFC][PATCH 1/9] tracing/events: Add 'signed' field to format
 files

On Tue, 2009-10-06 at 01:09 -0500, Tom Zanussi wrote:
> The sign info used for filters in the kernel is also useful to
> applications that process the trace stream.  Add it to the format
> files and make it available to userspace.
> 
> Signed-off-by: Tom Zanussi <tzanussi@...il.com>
> ---
>  include/trace/ftrace.h              |   15 +++++++++------
>  kernel/trace/ring_buffer.c          |   15 +++++++++------
>  kernel/trace/trace_events.c         |   24 ++++++++++++------------
>  kernel/trace/trace_export.c         |   25 ++++++++++++++-----------
>  kernel/trace/trace_syscalls.c       |   20 +++++++++++++-------
>  tools/perf/util/trace-event-parse.c |   24 ++++++++++++++++++++++++
>  tools/perf/util/trace-event.h       |    1 +
>  7 files changed, 82 insertions(+), 42 deletions(-)
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index cc0d966..c9bbcab 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -120,9 +120,10 @@
>  #undef __field
>  #define __field(type, item)					\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
> -			       "offset:%u;\tsize:%u;\n",		\
> +			       "offset:%u;\tsize:%u;\tsigned:%u;\n",	\
>  			       (unsigned int)offsetof(typeof(field), item), \
> -			       (unsigned int)sizeof(field.item));	\
> +			       (unsigned int)sizeof(field.item),	\
> +			       (unsigned int)is_signed_type(type));	\
>  	if (!ret)							\
>  		return 0;

I don't mind this change, but it makes me nervous. We really need to
solidify the output format file. This is adding a new field and will
already break the parsers in perf and trace_cmd.

Is there anything else that is needed? I really want to make sure that
we don't need to modify the output of the format files any more.

-- Steve

>  
> @@ -132,19 +133,21 @@
>  #undef __array
>  #define __array(type, item, len)						\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item "[" #len "];\t"	\
> -			       "offset:%u;\tsize:%u;\n",		\
> +			       "offset:%u;\tsize:%u;\tsigned:%u;\n",	\
>  			       (unsigned int)offsetof(typeof(field), item), \
> -			       (unsigned int)sizeof(field.item));	\
> +			       (unsigned int)sizeof(field.item),	\
> +			       (unsigned int)is_signed_type(type));	\
>  	if (!ret)							\
>  		return 0;
>  
>  #undef __dynamic_array
>  #define __dynamic_array(type, item, len)				       \
>  	ret = trace_seq_printf(s, "\tfield:__data_loc " #type "[] " #item ";\t"\
> -			       "offset:%u;\tsize:%u;\n",		       \
> +			       "offset:%u;\tsize:%u;\tsigned:%u;\n",	       \
>  			       (unsigned int)offsetof(typeof(field),	       \
>  					__data_loc_##item),		       \
> -			       (unsigned int)sizeof(field.__data_loc_##item)); \
> +			       (unsigned int)sizeof(field.__data_loc_##item), \
> +			       (unsigned int)is_signed_type(type));	\
>  	if (!ret)							       \
>  		return 0;
>  
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index d4ff019..e43c928 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -397,18 +397,21 @@ int ring_buffer_print_page_header(struct trace_seq *s)
>  	int ret;
>  
>  	ret = trace_seq_printf(s, "\tfield: u64 timestamp;\t"
> -			       "offset:0;\tsize:%u;\n",
> -			       (unsigned int)sizeof(field.time_stamp));
> +			       "offset:0;\tsize:%u;\tsigned:%u;\n",
> +			       (unsigned int)sizeof(field.time_stamp),
> +			       (unsigned int)is_signed_type(u64));
>  
>  	ret = trace_seq_printf(s, "\tfield: local_t commit;\t"
> -			       "offset:%u;\tsize:%u;\n",
> +			       "offset:%u;\tsize:%u;\tsigned:%u;\n",
>  			       (unsigned int)offsetof(typeof(field), commit),
> -			       (unsigned int)sizeof(field.commit));
> +			       (unsigned int)sizeof(field.commit),
> +			       (unsigned int)is_signed_type(long));
>  
>  	ret = trace_seq_printf(s, "\tfield: char data;\t"
> -			       "offset:%u;\tsize:%u;\n",
> +			       "offset:%u;\tsize:%u;\tsigned:%u;\n",
>  			       (unsigned int)offsetof(typeof(field), data),
> -			       (unsigned int)BUF_PAGE_SIZE);
> +			       (unsigned int)BUF_PAGE_SIZE,
> +			       (unsigned int)is_signed_type(char));
>  
>  	return ret;
>  }
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index d128f65..cf3cabf 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -507,7 +507,7 @@ extern char *__bad_type_size(void);
>  #define FIELD(type, name)						\
>  	sizeof(type) != sizeof(field.name) ? __bad_type_size() :	\
>  	#type, "common_" #name, offsetof(typeof(field), name),		\
> -		sizeof(field.name)
> +		sizeof(field.name), is_signed_type(type)
>  
>  static int trace_write_header(struct trace_seq *s)
>  {
> @@ -515,17 +515,17 @@ static int trace_write_header(struct trace_seq *s)
>  
>  	/* struct trace_entry */
>  	return trace_seq_printf(s,
> -				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -				"\n",
> -				FIELD(unsigned short, type),
> -				FIELD(unsigned char, flags),
> -				FIELD(unsigned char, preempt_count),
> -				FIELD(int, pid),
> -				FIELD(int, lock_depth));
> +			"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\tsigned:%u;\n"
> +			"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\tsigned:%u;\n"
> +			"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\tsigned:%u;\n"
> +			"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\tsigned:%u;\n"
> +			"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\tsigned:%u;\n"
> +			"\n",
> +			FIELD(unsigned short, type),
> +			FIELD(unsigned char, flags),
> +			FIELD(unsigned char, preempt_count),
> +			FIELD(int, pid),
> +			FIELD(int, lock_depth));
>  }
>  
>  static ssize_t
> diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
> index 9753fcc..31da218 100644
> --- a/kernel/trace/trace_export.c
> +++ b/kernel/trace/trace_export.c
> @@ -66,44 +66,47 @@ static void __used ____ftrace_check_##name(void)		\
>  #undef __field
>  #define __field(type, item)						\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
> -			       "offset:%zu;\tsize:%zu;\n",		\
> +			       "offset:%zu;\tsize:%zu;\tsigned:%u;\n",	\
>  			       offsetof(typeof(field), item),		\
> -			       sizeof(field.item));			\
> +			       sizeof(field.item), is_signed_type(type)); \
>  	if (!ret)							\
>  		return 0;
>  
>  #undef __field_desc
>  #define __field_desc(type, container, item)				\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
> -			       "offset:%zu;\tsize:%zu;\n",		\
> +			       "offset:%zu;\tsize:%zu;\tsigned:%u;\n",	\
>  			       offsetof(typeof(field), container.item),	\
> -			       sizeof(field.container.item));		\
> +			       sizeof(field.container.item),		\
> +			       is_signed_type(type));			\
>  	if (!ret)							\
>  		return 0;
>  
>  #undef __array
>  #define __array(type, item, len)					\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item "[" #len "];\t" \
> -			       "offset:%zu;\tsize:%zu;\n",		\
> -			       offsetof(typeof(field), item),	\
> -			       sizeof(field.item));		\
> +			       "offset:%zu;\tsize:%zu;\tsigned:%u;\n",	\
> +			       offsetof(typeof(field), item),		\
> +			       sizeof(field.item), is_signed_type(type)); \
>  	if (!ret)							\
>  		return 0;
>  
>  #undef __array_desc
>  #define __array_desc(type, container, item, len)			\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item "[" #len "];\t" \
> -			       "offset:%zu;\tsize:%zu;\n",		\
> +			       "offset:%zu;\tsize:%zu;\tsigned:%u;\n",	\
>  			       offsetof(typeof(field), container.item),	\
> -			       sizeof(field.container.item));		\
> +			       sizeof(field.container.item),		\
> +			       is_signed_type(type));			\
>  	if (!ret)							\
>  		return 0;
>  
>  #undef __dynamic_array
>  #define __dynamic_array(type, item)					\
>  	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
> -			       "offset:%zu;\tsize:0;\n",		\
> -			       offsetof(typeof(field), item));		\
> +			       "offset:%zu;\tsize:0;\tsigned:%u;\n",	\
> +			       offsetof(typeof(field), item),		\
> +			       is_signed_type(type));			\
>  	if (!ret)							\
>  		return 0;
>  
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 527e17e..d99abc4 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -103,7 +103,8 @@ extern char *__bad_type_size(void);
>  #define SYSCALL_FIELD(type, name)					\
>  	sizeof(type) != sizeof(trace.name) ?				\
>  		__bad_type_size() :					\
> -		#type, #name, offsetof(typeof(trace), name), sizeof(trace.name)
> +		#type, #name, offsetof(typeof(trace), name),		\
> +		sizeof(trace.name), is_signed_type(type)
>  
>  int syscall_enter_format(struct ftrace_event_call *call, struct trace_seq *s)
>  {
> @@ -120,7 +121,8 @@ int syscall_enter_format(struct ftrace_event_call *call, struct trace_seq *s)
>  	if (!entry)
>  		return 0;
>  
> -	ret = trace_seq_printf(s, "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
> +	ret = trace_seq_printf(s, "\tfield:%s %s;\toffset:%zu;\tsize:%zu;"
> +			       "\tsigned:%u;\n",
>  			       SYSCALL_FIELD(int, nr));
>  	if (!ret)
>  		return 0;
> @@ -130,8 +132,10 @@ int syscall_enter_format(struct ftrace_event_call *call, struct trace_seq *s)
>  				        entry->args[i]);
>  		if (!ret)
>  			return 0;
> -		ret = trace_seq_printf(s, "\toffset:%d;\tsize:%zu;\n", offset,
> -				       sizeof(unsigned long));
> +		ret = trace_seq_printf(s, "\toffset:%d;\tsize:%zu;"
> +				       "\tsigned:%u;\n", offset,
> +				       sizeof(unsigned long),
> +				       is_signed_type(unsigned long));
>  		if (!ret)
>  			return 0;
>  		offset += sizeof(unsigned long);
> @@ -163,8 +167,10 @@ int syscall_exit_format(struct ftrace_event_call *call, struct trace_seq *s)
>  	struct syscall_trace_exit trace;
>  
>  	ret = trace_seq_printf(s,
> -			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
> -			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n",
> +			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;"
> +			       "\tsigned:%u;\n"
> +			       "\tfield:%s %s;\toffset:%zu;\tsize:%zu;"
> +			       "\tsigned:%u;\n",
>  			       SYSCALL_FIELD(int, nr),
>  			       SYSCALL_FIELD(long, ret));
>  	if (!ret)
> @@ -212,7 +218,7 @@ int syscall_exit_define_fields(struct ftrace_event_call *call)
>  	if (ret)
>  		return ret;
>  
> -	ret = trace_define_field(call, SYSCALL_FIELD(long, ret), 0,
> +	ret = trace_define_field(call, SYSCALL_FIELD(long, ret),
>  				 FILTER_OTHER);
>  
>  	return ret;
> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> index 55b41b9..be8412d 100644
> --- a/tools/perf/util/trace-event-parse.c
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -897,6 +897,21 @@ static int event_read_fields(struct event *event, struct format_field **fields)
>  		if (read_expected(EVENT_OP, (char *)";") < 0)
>  			goto fail_expect;
>  
> +		if (read_expected(EVENT_ITEM, (char *)"signed") < 0)
> +			goto fail_expect;
> +
> +		if (read_expected(EVENT_OP, (char *)":") < 0)
> +			goto fail_expect;
> +
> +		if (read_expect_type(EVENT_ITEM, &token))
> +			goto fail;
> +		if (strtoul(token, NULL, 0))
> +			field->flags |= FIELD_IS_SIGNED;
> +		free_token(token);
> +
> +		if (read_expected(EVENT_OP, (char *)";") < 0)
> +			goto fail_expect;
> +
>  		if (read_expect_type(EVENT_NEWLINE, &token) < 0)
>  			goto fail;
>  		free_token(token);
> @@ -2845,6 +2860,15 @@ static void parse_header_field(char *type,
>  	free_token(token);
>  	if (read_expected(EVENT_OP, (char *)";") < 0)
>  		return;
> +	if (read_expected(EVENT_ITEM, (char *)"signed") < 0)
> +		return;
> +	if (read_expected(EVENT_OP, (char *)":") < 0)
> +		return;
> +	if (read_expect_type(EVENT_ITEM, &token) < 0)
> +		return;
> +	free_token(token);
> +	if (read_expected(EVENT_OP, (char *)";") < 0)
> +		return;
>  	if (read_expect_type(EVENT_NEWLINE, &token) < 0)
>  		return;
>  	free_token(token);
> diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
> index 162c3e6..00b440d 100644
> --- a/tools/perf/util/trace-event.h
> +++ b/tools/perf/util/trace-event.h
> @@ -26,6 +26,7 @@ enum {
>  enum format_flags {
>  	FIELD_IS_ARRAY		= 1,
>  	FIELD_IS_POINTER	= 2,
> +	FIELD_IS_SIGNED		= 4,
>  };
>  
>  struct format_field {

--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ