[PATCH 1/4] tracing: add run-time field descriptions for event filtering

[PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Tom Zanussi]

This patch makes the field descriptions defined for event tracing
available at run-time, for the event-filtering mechanism introduced in a
subsequent patch.

The common event fields are prepended with 'common_' in the format
display, allowing them to be distinguished from the other fields that
might internally have same name and can therefore be unambiguously used
in filters.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>

---
 kernel/trace/trace.h                |   30 ++++++++++++++++-------
 kernel/trace/trace_events.c         |   40 ++++++++++++++++++++++++++++++-
 kernel/trace/trace_events_stage_2.h |   45 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace_events_stage_3.h |    2 +
 4 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 7cfb741..9288dc7 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -775,16 +775,26 @@ enum {
 	TRACE_EVENT_TYPE_RAW		= 2,
 };
 
+struct ftrace_event_field {
+	struct list_head	link;
+	char			*name;
+	char			*type;
+	int			offset;
+	int			size;
+};
+
 struct ftrace_event_call {
-	char		*name;
-	char		*system;
-	struct dentry	*dir;
-	int		enabled;
-	int		(*regfunc)(void);
-	void		(*unregfunc)(void);
-	int		id;
-	int		(*raw_init)(void);
-	int		(*show_format)(struct trace_seq *s);
+	char			*name;
+	char			*system;
+	struct dentry		*dir;
+	int			enabled;
+	int			(*regfunc)(void);
+	void			(*unregfunc)(void);
+	int			id;
+	int			(*raw_init)(void);
+	int			(*show_format)(struct trace_seq *s);
+	int			(*define_fields)(void);
+	struct list_head	fields;
 
 #ifdef CONFIG_EVENT_PROFILE
 	atomic_t	profile_count;
@@ -793,6 +803,8 @@ struct ftrace_event_call {
 #endif
 };
 
+int trace_define_field(struct ftrace_event_call *call, char *type,
+		       char *name, int offset, int size);
 void event_trace_printk(unsigned long ip, const char *fmt, ...);
 extern struct ftrace_event_call __start_ftrace_events[];
 extern struct ftrace_event_call __stop_ftrace_events[];
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3047b56..961b057 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -19,6 +19,34 @@
 
 static DEFINE_MUTEX(event_mutex);
 
+int trace_define_field(struct ftrace_event_call *call, char *type,
+		       char *name, int offset, int size)
+{
+	struct ftrace_event_field *field;
+
+	field = kmalloc(sizeof(*field), GFP_KERNEL);
+	if (!field)
+		goto err;
+	field->name = kstrdup(name, GFP_KERNEL);
+	if (!field->name)
+		goto err;
+	field->type = kstrdup(type, GFP_KERNEL);
+	if (!field->type)
+		goto err;
+	field->offset = offset;
+	field->size = size;
+	list_add(&field->link, &call->fields);
+
+	return 0;
+err:
+	if (field) {
+		kfree(field->name);
+		kfree(field->type);
+	}
+	kfree(field);
+	return -ENOMEM;
+}
+
 static void ftrace_clear_events(void)
 {
 	struct ftrace_event_call *call = (void *)__start_ftrace_events;
@@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 #undef FIELD
 #define FIELD(type, name)						\
-	#type, #name, offsetof(typeof(field), name), sizeof(field.name)
+	#type, "common_" #name, offsetof(typeof(field), name),		\
+		sizeof(field.name)
 
 static int trace_write_header(struct trace_seq *s)
 {
@@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
 					call->name);
 	}
 
+	if (call->define_fields) {
+		ret = call->define_fields();
+		if (ret < 0) {
+			pr_warning("Could not initialize trace point"
+				   " events/%s\n", call->name);
+			return ret;
+		}
+	}
+
 	/* A trace may not want to export its format */
 	if (!call->show_format)
 		return 0;
diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
index 5117c43..30743f7 100644
--- a/kernel/trace/trace_events_stage_2.h
+++ b/kernel/trace/trace_events_stage_2.h
@@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s)				\
 }
 
 #include <trace/trace_event_types.h>
+
+#undef __field
+#define __field(type, item)						\
+	ret = trace_define_field(event_call, #type, #item,		\
+				 offsetof(typeof(field), item),		\
+				 sizeof(field.item));			\
+	if (ret)							\
+		return ret;
+
+#undef __array
+#define __array(type, item, len)					\
+	ret = trace_define_field(event_call, #type "[" #len "]", #item,	\
+				 offsetof(typeof(field), item),		\
+				 sizeof(field.item));			\
+	if (ret)							\
+		return ret;
+
+#define __common_field(type, item)					\
+	ret = trace_define_field(event_call, #type, "common_" #item,	\
+				 offsetof(typeof(field.ent), item),	\
+				 sizeof(field.ent.item));		\
+	if (ret)							\
+		return ret;
+
+#undef TRACE_EVENT
+#define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
+int									\
+ftrace_define_fields_##call(void)					\
+{									\
+	struct ftrace_raw_##call field;					\
+	struct ftrace_event_call *event_call = &event_##call;		\
+	int ret;							\
+									\
+	__common_field(unsigned char, type);				\
+	__common_field(unsigned char, flags);				\
+	__common_field(unsigned char, preempt_count);			\
+	__common_field(int, pid);					\
+	__common_field(int, tgid);					\
+									\
+	tstruct;							\
+									\
+	return ret;							\
+}
+
+#include <trace/trace_event_types.h>
diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
index 6b3261c..468938f 100644
--- a/kernel/trace/trace_events_stage_3.h
+++ b/kernel/trace/trace_events_stage_3.h
@@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void)				\
 	if (!id)							\
 		return -ENODEV;						\
 	event_##call.id = id;						\
+	INIT_LIST_HEAD(&event_##call.fields);				\
 	return 0;							\
 }									\
 									\
@@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.regfunc		= ftrace_raw_reg_event_##call,		\
 	.unregfunc		= ftrace_raw_unreg_event_##call,	\
 	.show_format		= ftrace_format_##call,			\
+	.define_fields		= ftrace_define_fields_##call,		\
 	_TRACE_PROFILE_INIT(call)					\
 }
 
-- 
1.5.6.3

Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Frederic Weisbecker]

Hi Tom,


On Sun, Mar 22, 2009 at 03:30:39AM -0500, Tom Zanussi wrote:
> This patch makes the field descriptions defined for event tracing
> available at run-time, for the event-filtering mechanism introduced in a
> subsequent patch.
> 
> The common event fields are prepended with 'common_' in the format
> display, allowing them to be distinguished from the other fields that
> might internally have same name and can therefore be unambiguously used
> in filters.
> 
> Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> 
> ---
>  kernel/trace/trace.h                |   30 ++++++++++++++++-------
>  kernel/trace/trace_events.c         |   40 ++++++++++++++++++++++++++++++-
>  kernel/trace/trace_events_stage_2.h |   45 +++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_events_stage_3.h |    2 +
>  4 files changed, 107 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 7cfb741..9288dc7 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -775,16 +775,26 @@ enum {
>  	TRACE_EVENT_TYPE_RAW		= 2,
>  };
>  
> +struct ftrace_event_field {
> +	struct list_head	link;
> +	char			*name;
> +	char			*type;
> +	int			offset;
> +	int			size;
> +};
> +
>  struct ftrace_event_call {
> -	char		*name;
> -	char		*system;
> -	struct dentry	*dir;
> -	int		enabled;
> -	int		(*regfunc)(void);
> -	void		(*unregfunc)(void);
> -	int		id;
> -	int		(*raw_init)(void);
> -	int		(*show_format)(struct trace_seq *s);
> +	char			*name;
> +	char			*system;
> +	struct dentry		*dir;
> +	int			enabled;
> +	int			(*regfunc)(void);
> +	void			(*unregfunc)(void);
> +	int			id;
> +	int			(*raw_init)(void);
> +	int			(*show_format)(struct trace_seq *s);
> +	int			(*define_fields)(void);
> +	struct list_head	fields;
>  
>  #ifdef CONFIG_EVENT_PROFILE
>  	atomic_t	profile_count;
> @@ -793,6 +803,8 @@ struct ftrace_event_call {
>  #endif
>  };
>  
> +int trace_define_field(struct ftrace_event_call *call, char *type,
> +		       char *name, int offset, int size);
>  void event_trace_printk(unsigned long ip, const char *fmt, ...);
>  extern struct ftrace_event_call __start_ftrace_events[];
>  extern struct ftrace_event_call __stop_ftrace_events[];
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 3047b56..961b057 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -19,6 +19,34 @@
>  
>  static DEFINE_MUTEX(event_mutex);
>  
> +int trace_define_field(struct ftrace_event_call *call, char *type,
> +		       char *name, int offset, int size)
> +{
> +	struct ftrace_event_field *field;
> +
> +	field = kmalloc(sizeof(*field), GFP_KERNEL);
> +	if (!field)
> +		goto err;
> +	field->name = kstrdup(name, GFP_KERNEL);
> +	if (!field->name)
> +		goto err;
> +	field->type = kstrdup(type, GFP_KERNEL);
> +	if (!field->type)
> +		goto err;
> +	field->offset = offset;
> +	field->size = size;
> +	list_add(&field->link, &call->fields);
> +
> +	return 0;
> +err:
> +	if (field) {
> +		kfree(field->name);
> +		kfree(field->type);


You need kzalloc to allocate field.
With kmalloc, field will point to random filled memory
after a fresh allocation.

Imagine this path:

if (!field->name)
	goto err;
...
err:
	kfree(field->name) <- field->name = NULL, correct
	kfree(field->type) <- field->type = ?


> +	}
> +	kfree(field);
> +	return -ENOMEM;
> +}
> +
>  static void ftrace_clear_events(void)
>  {
>  	struct ftrace_event_call *call = (void *)__start_ftrace_events;
> @@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  
>  #undef FIELD
>  #define FIELD(type, name)						\
> -	#type, #name, offsetof(typeof(field), name), sizeof(field.name)
> +	#type, "common_" #name, offsetof(typeof(field), name),		\
> +		sizeof(field.name)
>  
>  static int trace_write_header(struct trace_seq *s)
>  {
> @@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
>  					call->name);
>  	}
>  
> +	if (call->define_fields) {
> +		ret = call->define_fields();
> +		if (ret < 0) {
> +			pr_warning("Could not initialize trace point"
> +				   " events/%s\n", call->name);
> +			return ret;
> +		}
> +	}
> +
>  	/* A trace may not want to export its format */
>  	if (!call->show_format)
>  		return 0;
> diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
> index 5117c43..30743f7 100644
> --- a/kernel/trace/trace_events_stage_2.h
> +++ b/kernel/trace/trace_events_stage_2.h
> @@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s)				\
>  }
>  
>  #include <trace/trace_event_types.h>
> +
> +#undef __field
> +#define __field(type, item)						\
> +	ret = trace_define_field(event_call, #type, #item,		\
> +				 offsetof(typeof(field), item),		\
> +				 sizeof(field.item));			\
> +	if (ret)							\
> +		return ret;
> +
> +#undef __array
> +#define __array(type, item, len)					\
> +	ret = trace_define_field(event_call, #type "[" #len "]", #item,	\
> +				 offsetof(typeof(field), item),		\
> +				 sizeof(field.item));			\
> +	if (ret)							\
> +		return ret;
> +
> +#define __common_field(type, item)					\
> +	ret = trace_define_field(event_call, #type, "common_" #item,	\
> +				 offsetof(typeof(field.ent), item),	\
> +				 sizeof(field.ent.item));		\
> +	if (ret)							\
> +		return ret;
> +
> +#undef TRACE_EVENT
> +#define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
> +int									\
> +ftrace_define_fields_##call(void)					\
> +{									\
> +	struct ftrace_raw_##call field;					\
> +	struct ftrace_event_call *event_call = &event_##call;		\
> +	int ret;							\
> +									\
> +	__common_field(unsigned char, type);				\
> +	__common_field(unsigned char, flags);				\
> +	__common_field(unsigned char, preempt_count);			\
> +	__common_field(int, pid);					\
> +	__common_field(int, tgid);					\
> +									\
> +	tstruct;							\
> +									\
> +	return ret;							\
> +}
> +
> +#include <trace/trace_event_types.h>
> diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
> index 6b3261c..468938f 100644
> --- a/kernel/trace/trace_events_stage_3.h
> +++ b/kernel/trace/trace_events_stage_3.h
> @@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void)				\
>  	if (!id)							\
>  		return -ENODEV;						\
>  	event_##call.id = id;						\
> +	INIT_LIST_HEAD(&event_##call.fields);				\
>  	return 0;							\
>  }									\
>  									\
> @@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
>  	.regfunc		= ftrace_raw_reg_event_##call,		\
>  	.unregfunc		= ftrace_raw_unreg_event_##call,	\
>  	.show_format		= ftrace_format_##call,			\
> +	.define_fields		= ftrace_define_fields_##call,		\
>  	_TRACE_PROFILE_INIT(call)					\
>  }
>  
 
Other than the small possible memory leak, it looks good.

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Hi Tom,
> 
> 
> On Sun, Mar 22, 2009 at 03:30:39AM -0500, Tom Zanussi wrote:
> > This patch makes the field descriptions defined for event tracing
> > available at run-time, for the event-filtering mechanism introduced in a
> > subsequent patch.
> > 
> > The common event fields are prepended with 'common_' in the format
> > display, allowing them to be distinguished from the other fields that
> > might internally have same name and can therefore be unambiguously used
> > in filters.
> > 
> > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > 
> > ---
> >  kernel/trace/trace.h                |   30 ++++++++++++++++-------
> >  kernel/trace/trace_events.c         |   40 ++++++++++++++++++++++++++++++-
> >  kernel/trace/trace_events_stage_2.h |   45 +++++++++++++++++++++++++++++++++++
> >  kernel/trace/trace_events_stage_3.h |    2 +
> >  4 files changed, 107 insertions(+), 10 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 7cfb741..9288dc7 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -775,16 +775,26 @@ enum {
> >  	TRACE_EVENT_TYPE_RAW		= 2,
> >  };
> >  
> > +struct ftrace_event_field {
> > +	struct list_head	link;
> > +	char			*name;
> > +	char			*type;
> > +	int			offset;
> > +	int			size;
> > +};
> > +
> >  struct ftrace_event_call {
> > -	char		*name;
> > -	char		*system;
> > -	struct dentry	*dir;
> > -	int		enabled;
> > -	int		(*regfunc)(void);
> > -	void		(*unregfunc)(void);
> > -	int		id;
> > -	int		(*raw_init)(void);
> > -	int		(*show_format)(struct trace_seq *s);
> > +	char			*name;
> > +	char			*system;
> > +	struct dentry		*dir;
> > +	int			enabled;
> > +	int			(*regfunc)(void);
> > +	void			(*unregfunc)(void);
> > +	int			id;
> > +	int			(*raw_init)(void);
> > +	int			(*show_format)(struct trace_seq *s);
> > +	int			(*define_fields)(void);
> > +	struct list_head	fields;
> >  
> >  #ifdef CONFIG_EVENT_PROFILE
> >  	atomic_t	profile_count;
> > @@ -793,6 +803,8 @@ struct ftrace_event_call {
> >  #endif
> >  };
> >  
> > +int trace_define_field(struct ftrace_event_call *call, char *type,
> > +		       char *name, int offset, int size);
> >  void event_trace_printk(unsigned long ip, const char *fmt, ...);
> >  extern struct ftrace_event_call __start_ftrace_events[];
> >  extern struct ftrace_event_call __stop_ftrace_events[];
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 3047b56..961b057 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -19,6 +19,34 @@
> >  
> >  static DEFINE_MUTEX(event_mutex);
> >  
> > +int trace_define_field(struct ftrace_event_call *call, char *type,
> > +		       char *name, int offset, int size)
> > +{
> > +	struct ftrace_event_field *field;
> > +
> > +	field = kmalloc(sizeof(*field), GFP_KERNEL);
> > +	if (!field)
> > +		goto err;
> > +	field->name = kstrdup(name, GFP_KERNEL);
> > +	if (!field->name)
> > +		goto err;
> > +	field->type = kstrdup(type, GFP_KERNEL);
> > +	if (!field->type)
> > +		goto err;
> > +	field->offset = offset;
> > +	field->size = size;
> > +	list_add(&field->link, &call->fields);
> > +
> > +	return 0;
> > +err:
> > +	if (field) {
> > +		kfree(field->name);
> > +		kfree(field->type);
> 
> 
> You need kzalloc to allocate field.
> With kmalloc, field will point to random filled memory
> after a fresh allocation.
> 
> Imagine this path:
> 
> if (!field->name)
> 	goto err;
> ...
> err:
> 	kfree(field->name) <- field->name = NULL, correct
> 	kfree(field->type) <- field->type = ?
> 
> 
> > +	}
> > +	kfree(field);
> > +	return -ENOMEM;
> > +}
> > +
> >  static void ftrace_clear_events(void)
> >  {
> >  	struct ftrace_event_call *call = (void *)__start_ftrace_events;
> > @@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> >  
> >  #undef FIELD
> >  #define FIELD(type, name)						\
> > -	#type, #name, offsetof(typeof(field), name), sizeof(field.name)
> > +	#type, "common_" #name, offsetof(typeof(field), name),		\
> > +		sizeof(field.name)
> >  
> >  static int trace_write_header(struct trace_seq *s)
> >  {
> > @@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
> >  					call->name);
> >  	}
> >  
> > +	if (call->define_fields) {
> > +		ret = call->define_fields();
> > +		if (ret < 0) {
> > +			pr_warning("Could not initialize trace point"
> > +				   " events/%s\n", call->name);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	/* A trace may not want to export its format */
> >  	if (!call->show_format)
> >  		return 0;
> > diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
> > index 5117c43..30743f7 100644
> > --- a/kernel/trace/trace_events_stage_2.h
> > +++ b/kernel/trace/trace_events_stage_2.h
> > @@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s)				\
> >  }
> >  
> >  #include <trace/trace_event_types.h>
> > +
> > +#undef __field
> > +#define __field(type, item)						\
> > +	ret = trace_define_field(event_call, #type, #item,		\
> > +				 offsetof(typeof(field), item),		\
> > +				 sizeof(field.item));			\
> > +	if (ret)							\
> > +		return ret;
> > +
> > +#undef __array
> > +#define __array(type, item, len)					\
> > +	ret = trace_define_field(event_call, #type "[" #len "]", #item,	\
> > +				 offsetof(typeof(field), item),		\
> > +				 sizeof(field.item));			\
> > +	if (ret)							\
> > +		return ret;
> > +
> > +#define __common_field(type, item)					\
> > +	ret = trace_define_field(event_call, #type, "common_" #item,	\
> > +				 offsetof(typeof(field.ent), item),	\
> > +				 sizeof(field.ent.item));		\
> > +	if (ret)							\
> > +		return ret;
> > +
> > +#undef TRACE_EVENT
> > +#define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
> > +int									\
> > +ftrace_define_fields_##call(void)					\
> > +{									\
> > +	struct ftrace_raw_##call field;					\
> > +	struct ftrace_event_call *event_call = &event_##call;		\
> > +	int ret;							\
> > +									\
> > +	__common_field(unsigned char, type);				\
> > +	__common_field(unsigned char, flags);				\
> > +	__common_field(unsigned char, preempt_count);			\
> > +	__common_field(int, pid);					\
> > +	__common_field(int, tgid);					\
> > +									\
> > +	tstruct;							\
> > +									\
> > +	return ret;							\
> > +}
> > +
> > +#include <trace/trace_event_types.h>
> > diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
> > index 6b3261c..468938f 100644
> > --- a/kernel/trace/trace_events_stage_3.h
> > +++ b/kernel/trace/trace_events_stage_3.h
> > @@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void)				\
> >  	if (!id)							\
> >  		return -ENODEV;						\
> >  	event_##call.id = id;						\
> > +	INIT_LIST_HEAD(&event_##call.fields);				\
> >  	return 0;							\
> >  }									\
> >  									\
> > @@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
> >  	.regfunc		= ftrace_raw_reg_event_##call,		\
> >  	.unregfunc		= ftrace_raw_unreg_event_##call,	\
> >  	.show_format		= ftrace_format_##call,			\
> > +	.define_fields		= ftrace_define_fields_##call,		\
> >  	_TRACE_PROFILE_INIT(call)					\
> >  }
> >  
>  
> Other than the small possible memory leak, it looks good.

it's not a memory leak but an incorrect kfree(), which can crash the 
box.

> 
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

thanks - i also queued up a fix for the kfree() bug.

	Ingo

Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Frederic Weisbecker]

On Sun, Mar 22, 2009 at 06:44:43PM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Hi Tom,
> > 
> > 
> > On Sun, Mar 22, 2009 at 03:30:39AM -0500, Tom Zanussi wrote:
> > > This patch makes the field descriptions defined for event tracing
> > > available at run-time, for the event-filtering mechanism introduced in a
> > > subsequent patch.
> > > 
> > > The common event fields are prepended with 'common_' in the format
> > > display, allowing them to be distinguished from the other fields that
> > > might internally have same name and can therefore be unambiguously used
> > > in filters.
> > > 
> > > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > > 
> > > ---
> > >  kernel/trace/trace.h                |   30 ++++++++++++++++-------
> > >  kernel/trace/trace_events.c         |   40 ++++++++++++++++++++++++++++++-
> > >  kernel/trace/trace_events_stage_2.h |   45 +++++++++++++++++++++++++++++++++++
> > >  kernel/trace/trace_events_stage_3.h |    2 +
> > >  4 files changed, 107 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > > index 7cfb741..9288dc7 100644
> > > --- a/kernel/trace/trace.h
> > > +++ b/kernel/trace/trace.h
> > > @@ -775,16 +775,26 @@ enum {
> > >  	TRACE_EVENT_TYPE_RAW		= 2,
> > >  };
> > >  
> > > +struct ftrace_event_field {
> > > +	struct list_head	link;
> > > +	char			*name;
> > > +	char			*type;
> > > +	int			offset;
> > > +	int			size;
> > > +};
> > > +
> > >  struct ftrace_event_call {
> > > -	char		*name;
> > > -	char		*system;
> > > -	struct dentry	*dir;
> > > -	int		enabled;
> > > -	int		(*regfunc)(void);
> > > -	void		(*unregfunc)(void);
> > > -	int		id;
> > > -	int		(*raw_init)(void);
> > > -	int		(*show_format)(struct trace_seq *s);
> > > +	char			*name;
> > > +	char			*system;
> > > +	struct dentry		*dir;
> > > +	int			enabled;
> > > +	int			(*regfunc)(void);
> > > +	void			(*unregfunc)(void);
> > > +	int			id;
> > > +	int			(*raw_init)(void);
> > > +	int			(*show_format)(struct trace_seq *s);
> > > +	int			(*define_fields)(void);
> > > +	struct list_head	fields;
> > >  
> > >  #ifdef CONFIG_EVENT_PROFILE
> > >  	atomic_t	profile_count;
> > > @@ -793,6 +803,8 @@ struct ftrace_event_call {
> > >  #endif
> > >  };
> > >  
> > > +int trace_define_field(struct ftrace_event_call *call, char *type,
> > > +		       char *name, int offset, int size);
> > >  void event_trace_printk(unsigned long ip, const char *fmt, ...);
> > >  extern struct ftrace_event_call __start_ftrace_events[];
> > >  extern struct ftrace_event_call __stop_ftrace_events[];
> > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > > index 3047b56..961b057 100644
> > > --- a/kernel/trace/trace_events.c
> > > +++ b/kernel/trace/trace_events.c
> > > @@ -19,6 +19,34 @@
> > >  
> > >  static DEFINE_MUTEX(event_mutex);
> > >  
> > > +int trace_define_field(struct ftrace_event_call *call, char *type,
> > > +		       char *name, int offset, int size)
> > > +{
> > > +	struct ftrace_event_field *field;
> > > +
> > > +	field = kmalloc(sizeof(*field), GFP_KERNEL);
> > > +	if (!field)
> > > +		goto err;
> > > +	field->name = kstrdup(name, GFP_KERNEL);
> > > +	if (!field->name)
> > > +		goto err;
> > > +	field->type = kstrdup(type, GFP_KERNEL);
> > > +	if (!field->type)
> > > +		goto err;
> > > +	field->offset = offset;
> > > +	field->size = size;
> > > +	list_add(&field->link, &call->fields);
> > > +
> > > +	return 0;
> > > +err:
> > > +	if (field) {
> > > +		kfree(field->name);
> > > +		kfree(field->type);
> > 
> > 
> > You need kzalloc to allocate field.
> > With kmalloc, field will point to random filled memory
> > after a fresh allocation.
> > 
> > Imagine this path:
> > 
> > if (!field->name)
> > 	goto err;
> > ...
> > err:
> > 	kfree(field->name) <- field->name = NULL, correct
> > 	kfree(field->type) <- field->type = ?
> > 
> > 
> > > +	}
> > > +	kfree(field);
> > > +	return -ENOMEM;
> > > +}
> > > +
> > >  static void ftrace_clear_events(void)
> > >  {
> > >  	struct ftrace_event_call *call = (void *)__start_ftrace_events;
> > > @@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> > >  
> > >  #undef FIELD
> > >  #define FIELD(type, name)						\
> > > -	#type, #name, offsetof(typeof(field), name), sizeof(field.name)
> > > +	#type, "common_" #name, offsetof(typeof(field), name),		\
> > > +		sizeof(field.name)
> > >  
> > >  static int trace_write_header(struct trace_seq *s)
> > >  {
> > > @@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
> > >  					call->name);
> > >  	}
> > >  
> > > +	if (call->define_fields) {
> > > +		ret = call->define_fields();
> > > +		if (ret < 0) {
> > > +			pr_warning("Could not initialize trace point"
> > > +				   " events/%s\n", call->name);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >  	/* A trace may not want to export its format */
> > >  	if (!call->show_format)
> > >  		return 0;
> > > diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
> > > index 5117c43..30743f7 100644
> > > --- a/kernel/trace/trace_events_stage_2.h
> > > +++ b/kernel/trace/trace_events_stage_2.h
> > > @@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s)				\
> > >  }
> > >  
> > >  #include <trace/trace_event_types.h>
> > > +
> > > +#undef __field
> > > +#define __field(type, item)						\
> > > +	ret = trace_define_field(event_call, #type, #item,		\
> > > +				 offsetof(typeof(field), item),		\
> > > +				 sizeof(field.item));			\
> > > +	if (ret)							\
> > > +		return ret;
> > > +
> > > +#undef __array
> > > +#define __array(type, item, len)					\
> > > +	ret = trace_define_field(event_call, #type "[" #len "]", #item,	\
> > > +				 offsetof(typeof(field), item),		\
> > > +				 sizeof(field.item));			\
> > > +	if (ret)							\
> > > +		return ret;
> > > +
> > > +#define __common_field(type, item)					\
> > > +	ret = trace_define_field(event_call, #type, "common_" #item,	\
> > > +				 offsetof(typeof(field.ent), item),	\
> > > +				 sizeof(field.ent.item));		\
> > > +	if (ret)							\
> > > +		return ret;
> > > +
> > > +#undef TRACE_EVENT
> > > +#define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
> > > +int									\
> > > +ftrace_define_fields_##call(void)					\
> > > +{									\
> > > +	struct ftrace_raw_##call field;					\
> > > +	struct ftrace_event_call *event_call = &event_##call;		\
> > > +	int ret;							\
> > > +									\
> > > +	__common_field(unsigned char, type);				\
> > > +	__common_field(unsigned char, flags);				\
> > > +	__common_field(unsigned char, preempt_count);			\
> > > +	__common_field(int, pid);					\
> > > +	__common_field(int, tgid);					\
> > > +									\
> > > +	tstruct;							\
> > > +									\
> > > +	return ret;							\
> > > +}
> > > +
> > > +#include <trace/trace_event_types.h>
> > > diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
> > > index 6b3261c..468938f 100644
> > > --- a/kernel/trace/trace_events_stage_3.h
> > > +++ b/kernel/trace/trace_events_stage_3.h
> > > @@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void)				\
> > >  	if (!id)							\
> > >  		return -ENODEV;						\
> > >  	event_##call.id = id;						\
> > > +	INIT_LIST_HEAD(&event_##call.fields);				\
> > >  	return 0;							\
> > >  }									\
> > >  									\
> > > @@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
> > >  	.regfunc		= ftrace_raw_reg_event_##call,		\
> > >  	.unregfunc		= ftrace_raw_unreg_event_##call,	\
> > >  	.show_format		= ftrace_format_##call,			\
> > > +	.define_fields		= ftrace_define_fields_##call,		\
> > >  	_TRACE_PROFILE_INIT(call)					\
> > >  }
> > >  
> >  
> > Other than the small possible memory leak, it looks good.
> 
> it's not a memory leak but an incorrect kfree(), which can crash the 
> box.


Yeah, indeed.

 
> > 
> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> thanks - i also queued up a fix for the kfree() bug.
> 
> 	Ingo


Thanks.

[tip:tracing/filters] tracing: add run-time field descriptions for event filtering

[Tom Zanussi]

Commit-ID:  cf027f645e6aee4f0ca6197a6b6a57f327fdb13f
Gitweb:     http://git.kernel.org/tip/cf027f645e6aee4f0ca6197a6b6a57f327fdb13f
Author:     Tom Zanussi <tzanussi@gmail.com>
AuthorDate: Sun, 22 Mar 2009 03:30:39 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 22 Mar 2009 18:11:22 +0100

tracing: add run-time field descriptions for event filtering

This patch makes the field descriptions defined for event tracing
available at run-time, for the event-filtering mechanism introduced
in a subsequent patch.

The common event fields are prepended with 'common_' in the format
display, allowing them to be distinguished from the other fields
that might internally have same name and can therefore be
unambiguously used in filters.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1237710639.7703.46.camel@charm-linux>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace.h                |   30 ++++++++++++++++-------
 kernel/trace/trace_events.c         |   40 ++++++++++++++++++++++++++++++-
 kernel/trace/trace_events_stage_2.h |   45 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace_events_stage_3.h |    2 +
 4 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 7cfb741..9288dc7 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -775,16 +775,26 @@ enum {
 	TRACE_EVENT_TYPE_RAW		= 2,
 };
 
+struct ftrace_event_field {
+	struct list_head	link;
+	char			*name;
+	char			*type;
+	int			offset;
+	int			size;
+};
+
 struct ftrace_event_call {
-	char		*name;
-	char		*system;
-	struct dentry	*dir;
-	int		enabled;
-	int		(*regfunc)(void);
-	void		(*unregfunc)(void);
-	int		id;
-	int		(*raw_init)(void);
-	int		(*show_format)(struct trace_seq *s);
+	char			*name;
+	char			*system;
+	struct dentry		*dir;
+	int			enabled;
+	int			(*regfunc)(void);
+	void			(*unregfunc)(void);
+	int			id;
+	int			(*raw_init)(void);
+	int			(*show_format)(struct trace_seq *s);
+	int			(*define_fields)(void);
+	struct list_head	fields;
 
 #ifdef CONFIG_EVENT_PROFILE
 	atomic_t	profile_count;
@@ -793,6 +803,8 @@ struct ftrace_event_call {
 #endif
 };
 
+int trace_define_field(struct ftrace_event_call *call, char *type,
+		       char *name, int offset, int size);
 void event_trace_printk(unsigned long ip, const char *fmt, ...);
 extern struct ftrace_event_call __start_ftrace_events[];
 extern struct ftrace_event_call __stop_ftrace_events[];
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 3047b56..961b057 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -19,6 +19,34 @@
 
 static DEFINE_MUTEX(event_mutex);
 
+int trace_define_field(struct ftrace_event_call *call, char *type,
+		       char *name, int offset, int size)
+{
+	struct ftrace_event_field *field;
+
+	field = kmalloc(sizeof(*field), GFP_KERNEL);
+	if (!field)
+		goto err;
+	field->name = kstrdup(name, GFP_KERNEL);
+	if (!field->name)
+		goto err;
+	field->type = kstrdup(type, GFP_KERNEL);
+	if (!field->type)
+		goto err;
+	field->offset = offset;
+	field->size = size;
+	list_add(&field->link, &call->fields);
+
+	return 0;
+err:
+	if (field) {
+		kfree(field->name);
+		kfree(field->type);
+	}
+	kfree(field);
+	return -ENOMEM;
+}
+
 static void ftrace_clear_events(void)
 {
 	struct ftrace_event_call *call = (void *)__start_ftrace_events;
@@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 #undef FIELD
 #define FIELD(type, name)						\
-	#type, #name, offsetof(typeof(field), name), sizeof(field.name)
+	#type, "common_" #name, offsetof(typeof(field), name),		\
+		sizeof(field.name)
 
 static int trace_write_header(struct trace_seq *s)
 {
@@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
 					call->name);
 	}
 
+	if (call->define_fields) {
+		ret = call->define_fields();
+		if (ret < 0) {
+			pr_warning("Could not initialize trace point"
+				   " events/%s\n", call->name);
+			return ret;
+		}
+	}
+
 	/* A trace may not want to export its format */
 	if (!call->show_format)
 		return 0;
diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
index 5117c43..30743f7 100644
--- a/kernel/trace/trace_events_stage_2.h
+++ b/kernel/trace/trace_events_stage_2.h
@@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s)				\
 }
 
 #include <trace/trace_event_types.h>
+
+#undef __field
+#define __field(type, item)						\
+	ret = trace_define_field(event_call, #type, #item,		\
+				 offsetof(typeof(field), item),		\
+				 sizeof(field.item));			\
+	if (ret)							\
+		return ret;
+
+#undef __array
+#define __array(type, item, len)					\
+	ret = trace_define_field(event_call, #type "[" #len "]", #item,	\
+				 offsetof(typeof(field), item),		\
+				 sizeof(field.item));			\
+	if (ret)							\
+		return ret;
+
+#define __common_field(type, item)					\
+	ret = trace_define_field(event_call, #type, "common_" #item,	\
+				 offsetof(typeof(field.ent), item),	\
+				 sizeof(field.ent.item));		\
+	if (ret)							\
+		return ret;
+
+#undef TRACE_EVENT
+#define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
+int									\
+ftrace_define_fields_##call(void)					\
+{									\
+	struct ftrace_raw_##call field;					\
+	struct ftrace_event_call *event_call = &event_##call;		\
+	int ret;							\
+									\
+	__common_field(unsigned char, type);				\
+	__common_field(unsigned char, flags);				\
+	__common_field(unsigned char, preempt_count);			\
+	__common_field(int, pid);					\
+	__common_field(int, tgid);					\
+									\
+	tstruct;							\
+									\
+	return ret;							\
+}
+
+#include <trace/trace_event_types.h>
diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
index 6b3261c..468938f 100644
--- a/kernel/trace/trace_events_stage_3.h
+++ b/kernel/trace/trace_events_stage_3.h
@@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void)				\
 	if (!id)							\
 		return -ENODEV;						\
 	event_##call.id = id;						\
+	INIT_LIST_HEAD(&event_##call.fields);				\
 	return 0;							\
 }									\
 									\
@@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.regfunc		= ftrace_raw_reg_event_##call,		\
 	.unregfunc		= ftrace_raw_unreg_event_##call,	\
 	.show_format		= ftrace_format_##call,			\
+	.define_fields		= ftrace_define_fields_##call,		\
 	_TRACE_PROFILE_INIT(call)					\
 }
 

[tip:tracing/filters] tracing: add run-time field descriptions for event filtering, kfree fix

[Ingo Molnar]

Commit-ID:  fe9f57f250ab4d781b99504caeb218ca2db14c1a
Gitweb:     http://git.kernel.org/tip/fe9f57f250ab4d781b99504caeb218ca2db14c1a
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Sun, 22 Mar 2009 18:41:59 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 22 Mar 2009 18:43:25 +0100

tracing: add run-time field descriptions for event filtering, kfree fix

Impact: fix potential kfree of random data in (rare) failure path

Zero-initialize the field structure.

Reported-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <1237710639.7703.46.camel@charm-linux>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/trace/trace_events.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 97d4daa..594d78a 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -24,26 +24,31 @@ int trace_define_field(struct ftrace_event_call *call, char *type,
 {
 	struct ftrace_event_field *field;
 
-	field = kmalloc(sizeof(*field), GFP_KERNEL);
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
 	if (!field)
 		goto err;
+
 	field->name = kstrdup(name, GFP_KERNEL);
 	if (!field->name)
 		goto err;
+
 	field->type = kstrdup(type, GFP_KERNEL);
 	if (!field->type)
 		goto err;
+
 	field->offset = offset;
 	field->size = size;
 	list_add(&field->link, &call->fields);
 
 	return 0;
+
 err:
 	if (field) {
 		kfree(field->name);
 		kfree(field->type);
 	}
 	kfree(field);
+
 	return -ENOMEM;
 }
 

Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Steven Rostedt]

On Sun, 22 Mar 2009, Tom Zanussi wrote:
> @@ -19,6 +19,34 @@
>  
>  static DEFINE_MUTEX(event_mutex);
>  
> +int trace_define_field(struct ftrace_event_call *call, char *type,
> +		       char *name, int offset, int size)
> +{
> +	struct ftrace_event_field *field;
> +
> +	field = kmalloc(sizeof(*field), GFP_KERNEL);
> +	if (!field)
> +		goto err;
> +	field->name = kstrdup(name, GFP_KERNEL);
> +	if (!field->name)
> +		goto err;
> +	field->type = kstrdup(type, GFP_KERNEL);
> +	if (!field->type)
> +		goto err;
> +	field->offset = offset;
> +	field->size = size;
> +	list_add(&field->link, &call->fields);
> +
> +	return 0;
> +err:
> +	if (field) {
> +		kfree(field->name);
> +		kfree(field->type);

Field was not allocated with kzalloc, if we failed to allocate name, then 
type is unknown, and kfree(field->type) may corrupt the system.

What I would do is:

	field = kzalloc(...);
	if (!field)
		return -ENOMEM;

And then we could also get rid of the if (field) check in err.


> +	}
> +	kfree(field);
> +	return -ENOMEM;
> +}
> +

-- Steve

Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering

[Steven Rostedt]

On Sun, 22 Mar 2009, Frederic Weisbecker wrote:
> >  static DEFINE_MUTEX(event_mutex);
> >  
> > +int trace_define_field(struct ftrace_event_call *call, char *type,
> > +		       char *name, int offset, int size)
> > +{
> > +	struct ftrace_event_field *field;
> > +
> > +	field = kmalloc(sizeof(*field), GFP_KERNEL);
> > +	if (!field)
> > +		goto err;
> > +	field->name = kstrdup(name, GFP_KERNEL);
> > +	if (!field->name)
> > +		goto err;
> > +	field->type = kstrdup(type, GFP_KERNEL);
> > +	if (!field->type)
> > +		goto err;
> > +	field->offset = offset;
> > +	field->size = size;
> > +	list_add(&field->link, &call->fields);
> > +
> > +	return 0;
> > +err:
> > +	if (field) {
> > +		kfree(field->name);
> > +		kfree(field->type);
> 
> 
> You need kzalloc to allocate field.
> With kmalloc, field will point to random filled memory
> after a fresh allocation.
> 
> Imagine this path:
> 
> if (!field->name)
> 	goto err;
> ...
> err:
> 	kfree(field->name) <- field->name = NULL, correct
> 	kfree(field->type) <- field->type = ?
> 

Looks like Frederic got to you first ;-)

-- Steve

> 
> > +	}
> > +	kfree(field);
> > +	return -ENOMEM;
> > +}
> > +