Re: [PATCH tracing/kprobes 2/6] ftrace: Fix trace_add_event_call() to initialize list

Re: [PATCH tracing/kprobes 2/6] ftrace: Fix trace_add_event_call() to initialize list

[Steven Rostedt]

On Mon, 2009-09-14 at 16:49 -0400, Masami Hiramatsu wrote:
> Initialize event_call.list and handle failuer path in trace_add_event_call()
> for fixing below bug which occurred when I tried to add invalid event twice.
> 
> Could not create debugfs 'kmalloc' directory
> Failed to register kprobe event: kmalloc
> Faild to register probe event(-1)
> ------------[ cut here ]------------
> WARNING: at /home/mhiramat/ksrc/random-tracing/lib/list_debug.c:26
> __list_add+0x27/0x5c()
> Hardware name:
> list_add corruption. next->prev should be prev (c07d78cc), but was
> 00001000. (next=d854236c).
> Modules linked in: sunrpc uinput virtio_net virtio_balloon i2c_piix4 pcspkr
> i2c_core virtio_blk virtio_pci virtio_ring virtio [last unloaded:
> scsi_wait_scan]
> Pid: 1394, comm: tee Not tainted 2.6.31-rc9 #51
> Call Trace:
>  [<c0438424>] warn_slowpath_common+0x65/0x7c
>  [<c05371b3>] ? __list_add+0x27/0x5c
>  [<c043846f>] warn_slowpath_fmt+0x24/0x27
>  [<c05371b3>] __list_add+0x27/0x5c
>  [<c047f050>] list_add+0xa/0xc
>  [<c047f8f5>] trace_add_event_call+0x60/0x97
>  [<c0483133>] command_trace_probe+0x42c/0x51b
>  [<c044a1b3>] ? remove_wait_queue+0x22/0x27
>  [<c042a9c0>] ? __wake_up+0x32/0x3b
>  [<c04832f6>] probes_write+0xd4/0x10a
>  [<c0483222>] ? probes_write+0x0/0x10a
>  [<c04b27a9>] vfs_write+0x80/0xdf
>  [<c04b289c>] sys_write+0x3b/0x5d
>  [<c0670d41>] syscall_call+0x7/0xb
> ---[ end trace 2b962b5dc1fdc07d ]---
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> Cc: Jim Keniston <jkenisto@us.ibm.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Frank Ch. Eigler <fche@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Jason Baron <jbaron@redhat.com>
> Cc: K.Prasad <prasad@linux.vnet.ibm.com>
> Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Tom Zanussi <tzanussi@gmail.com>
> ---
> 
>  kernel/trace/trace_events.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index ba34920..38e82a5 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1009,10 +1009,14 @@ static int __trace_add_event_call(struct ftrace_event_call *call)
>  	if (!d_events)
>  		return -ENOENT;
>  
> +	INIT_LIST_HEAD(&call->list);

The INIT_LIST_HEAD is not needed here. The list_add will assign it.


>  	list_add(&call->list, &ftrace_events);
> -	return event_create_dir(call, d_events, &ftrace_event_id_fops,
> +	ret = event_create_dir(call, d_events, &ftrace_event_id_fops,
>  				&ftrace_enable_fops, &ftrace_event_filter_fops,
>  				&ftrace_event_format_fops);
> +	if (ret < 0)
> +		list_del(&call->list);

But this I can see is needed ;-)

-- Steve

> +	return ret;
>  }
>  
>  /* Add an additional event_call dynamically */
> 
> 

Re: [PATCH tracing/kprobes 3/6] ftrace: Fix trace_remove_event_call() to lock trace_event_mutex

[Steven Rostedt]

On Mon, 2009-09-14 at 16:49 -0400, Masami Hiramatsu wrote:

> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 38e82a5..a3d6bad 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1055,6 +1055,9 @@ static void remove_subsystem_dir(const char *name)
>  	}
>  }
>  
> +/*
> + * Must be called under locking both of event_mutex and trace_event_mutex.
> + */
>  static void __trace_remove_event_call(struct ftrace_event_call *call)
>  {
>  	ftrace_event_enable_disable(call, 0);
> @@ -1071,7 +1074,9 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
>  void trace_remove_event_call(struct ftrace_event_call *call)

Is this from a previous patch set, because I can't find this in either
my tree or tip/master.

-- Steve

>  {
>  	mutex_lock(&event_mutex);
> +	down_write(&trace_event_mutex);
>  	__trace_remove_event_call(call);
> +	up_write(&trace_event_mutex);
>  	mutex_unlock(&event_mutex);
>  }
>  
> 
> 

Re: [PATCH tracing/kprobes 3/6] ftrace: Fix trace_remove_event_call() to lock trace_event_mutex

[Frederic Weisbecker]

On Tue, Sep 15, 2009 at 11:17:30PM -0400, Steven Rostedt wrote:
> On Mon, 2009-09-14 at 16:49 -0400, Masami Hiramatsu wrote:
> 
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 38e82a5..a3d6bad 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -1055,6 +1055,9 @@ static void remove_subsystem_dir(const char *name)
> >  	}
> >  }
> >  
> > +/*
> > + * Must be called under locking both of event_mutex and trace_event_mutex.
> > + */
> >  static void __trace_remove_event_call(struct ftrace_event_call *call)
> >  {
> >  	ftrace_event_enable_disable(call, 0);
> > @@ -1071,7 +1074,9 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
> >  void trace_remove_event_call(struct ftrace_event_call *call)
> 
> Is this from a previous patch set, because I can't find this in either
> my tree or tip/master.
> 
> -- Steve


It's in tip:/tracing/kprobes, which is not merged into tip/master.
I have some pending updates in my tracing/kprobes tree:

Frederic Weisbecker (1):
      Merge commit 'tracing/core' into tracing/kprobes

Masami Hiramatsu (16):
      kprobes/x86: Call BUG() when reentering probe into KPROBES_HIT_SS
      kprobes/x86-64: Allow to reenter probe on post_handler
      kprobes/x86: Fix to add __kprobes to in-kernel fault handing functions
      kprobes: Fix to add __kprobes to notify_die
      kprobes/x86-64: Fix to move common_interrupt to .kprobes.text
      kprobes: Prohibit to probe native_get_debugreg
      x86: Allow x86-32 instruction decoder selftest on x86-64
      x86: Remove unused config macros from instruction decoder selftest
      x86: Add MMX support for instruction decoder
      kprobes/x86-32: Move irq-exit functions to kprobes section
      x86/ptrace: Fix regs_get_argument_nth() to add correct offset
      tracing/kprobes: Fix probe offset to be unsigned
      tracing/kprobes: Cleanup kprobe tracer code.
      tracing/kprobes: Add event profiling support
      tracing/kprobes: Add argument name support
      tracing/kprobes: Show event name in trace output

Plus the current set that I'm about to apply, if no problem arises.
I should also merge one more time tip:tracing/core into it because
it has developed another conflicts against it since then.

Ingo, if you are fine with it, I'll send you a pull request
soon.

Thanks,
Frederic.

Re: [PATCH tracing/kprobes 3/6] ftrace: Fix trace_remove_event_call() to lock trace_event_mutex

[Ananth N Mavinakayanahalli]

On Wed, Sep 16, 2009 at 06:05:12AM +0200, Frederic Weisbecker wrote:
> On Tue, Sep 15, 2009 at 11:17:30PM -0400, Steven Rostedt wrote:
> > On Mon, 2009-09-14 at 16:49 -0400, Masami Hiramatsu wrote:

...
 
> It's in tip:/tracing/kprobes, which is not merged into tip/master.
> I have some pending updates in my tracing/kprobes tree:
> 
> Frederic Weisbecker (1):
>       Merge commit 'tracing/core' into tracing/kprobes
> 
> Masami Hiramatsu (16):
>       kprobes/x86: Call BUG() when reentering probe into KPROBES_HIT_SS
>       kprobes/x86-64: Allow to reenter probe on post_handler
>       kprobes/x86: Fix to add __kprobes to in-kernel fault handing functions
>       kprobes: Fix to add __kprobes to notify_die
>       kprobes/x86-64: Fix to move common_interrupt to .kprobes.text
>       kprobes: Prohibit to probe native_get_debugreg
>       x86: Allow x86-32 instruction decoder selftest on x86-64
>       x86: Remove unused config macros from instruction decoder selftest
>       x86: Add MMX support for instruction decoder
>       kprobes/x86-32: Move irq-exit functions to kprobes section
>       x86/ptrace: Fix regs_get_argument_nth() to add correct offset
>       tracing/kprobes: Fix probe offset to be unsigned
>       tracing/kprobes: Cleanup kprobe tracer code.
>       tracing/kprobes: Add event profiling support
>       tracing/kprobes: Add argument name support
>       tracing/kprobes: Show event name in trace output
> 
> Plus the current set that I'm about to apply, if no problem arises.
> I should also merge one more time tip:tracing/core into it because
> it has developed another conflicts against it since then.
> 
> Ingo, if you are fine with it, I'll send you a pull request
> soon.

Frederic,

Could you please add http://lkml.org/lkml/2009/9/15/13 too?

Ananth

Re: [PATCH tracing/kprobes 3/6] ftrace: Fix trace_remove_event_call() to lock trace_event_mutex

[Frederic Weisbecker]

On Wed, Sep 16, 2009 at 10:24:20AM +0530, Ananth N Mavinakayanahalli wrote:
> On Wed, Sep 16, 2009 at 06:05:12AM +0200, Frederic Weisbecker wrote:
> > On Tue, Sep 15, 2009 at 11:17:30PM -0400, Steven Rostedt wrote:
> > > On Mon, 2009-09-14 at 16:49 -0400, Masami Hiramatsu wrote:
> 
> ...
>  
> > It's in tip:/tracing/kprobes, which is not merged into tip/master.
> > I have some pending updates in my tracing/kprobes tree:
> > 
> > Frederic Weisbecker (1):
> >       Merge commit 'tracing/core' into tracing/kprobes
> > 
> > Masami Hiramatsu (16):
> >       kprobes/x86: Call BUG() when reentering probe into KPROBES_HIT_SS
> >       kprobes/x86-64: Allow to reenter probe on post_handler
> >       kprobes/x86: Fix to add __kprobes to in-kernel fault handing functions
> >       kprobes: Fix to add __kprobes to notify_die
> >       kprobes/x86-64: Fix to move common_interrupt to .kprobes.text
> >       kprobes: Prohibit to probe native_get_debugreg
> >       x86: Allow x86-32 instruction decoder selftest on x86-64
> >       x86: Remove unused config macros from instruction decoder selftest
> >       x86: Add MMX support for instruction decoder
> >       kprobes/x86-32: Move irq-exit functions to kprobes section
> >       x86/ptrace: Fix regs_get_argument_nth() to add correct offset
> >       tracing/kprobes: Fix probe offset to be unsigned
> >       tracing/kprobes: Cleanup kprobe tracer code.
> >       tracing/kprobes: Add event profiling support
> >       tracing/kprobes: Add argument name support
> >       tracing/kprobes: Show event name in trace output
> > 
> > Plus the current set that I'm about to apply, if no problem arises.
> > I should also merge one more time tip:tracing/core into it because
> > it has developed another conflicts against it since then.
> > 
> > Ingo, if you are fine with it, I'll send you a pull request
> > soon.
> 
> Frederic,
> 
> Could you please add http://lkml.org/lkml/2009/9/15/13 too?
> 
> Ananth


Sure, I put it in the queue, thanks!

Re: [PATCH tracing/kprobes 2/6] ftrace: Fix trace_add_event_call() to initialize list

[Steven Rostedt]

On Wed, 2009-09-16 at 10:17 -0400, Masami Hiramatsu wrote:
> Steven Rostedt wrote:

> >> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> >> index ba34920..38e82a5 100644
> >> --- a/kernel/trace/trace_events.c
> >> +++ b/kernel/trace/trace_events.c
> >> @@ -1009,10 +1009,14 @@ static int __trace_add_event_call(struct ftrace_event_call *call)
> >>  	if (!d_events)
> >>  		return -ENOENT;
> >>  
> >> +	INIT_LIST_HEAD(&call->list);
> > 
> > The INIT_LIST_HEAD is not needed here. The list_add will assign it.
> 
> Without initializing it, list debugging code warns always :-)
> Please see, __list_add()@lib/list_debug.c

/me looks

from: include/linux/list.h

static inline void list_add(struct list_head *new, struct list_head *head)
{
	__list_add(new, head, head->next);
}

from: lib/list_debug.c

void __list_add(struct list_head *new,
			      struct list_head *prev,
			      struct list_head *next)
{
	WARN(next->prev != prev,
		"list_add corruption. next->prev should be "
		"prev (%p), but was %p. (next=%p).\n",
		prev, next->prev, next);
	WARN(prev->next != next,
		"list_add corruption. prev->next should be "
		"next (%p), but was %p. (prev=%p).\n",
		next, prev->next, prev);
	next->prev = new;
	new->next = next;
	new->prev = prev;
	prev->next = new;
}

What you pass in is:

        list_add(&call->list, &ftrace_events);

new = &call->list;
prev = &ftrace_events->prev;
next = &ftrace_events->next;

The above code never tests "new". The INIT_LIST_HEAD is useless.

-- Steve

Re: [PATCH tracing/kprobes 2/6] ftrace: Fix trace_add_event_call() to initialize list take 2

[Steven Rostedt]

On Wed, 2009-09-16 at 11:42 -0400, Masami Hiramatsu wrote:

> Oh, I see, I misunderstood. Thank you for pointing it out. :)
> Here, I updated the patch.

Thanks!

Acked-by: Steven Rostedt <rostedt@goodmis.org> for the whole series.

-- Steve

> 
> ---
> ftrace: Fix trace_add_event_call() to initialize list
> 
> From: Masami Hiramatsu <mhiramat@redhat.com>
> 
> Handle failuer path in trace_add_event_call() for fixing below bug
> which occurred when I tried to add invalid event twice.
> 
> Could not create debugfs 'kmalloc' directory
> Failed to register kprobe event: kmalloc
> Faild to register probe event(-1)
> ------------[ cut here ]------------
> WARNING: at /home/mhiramat/ksrc/random-tracing/lib/list_debug.c:26
> __list_add+0x27/0x5c()
> Hardware name:
> list_add corruption. next->prev should be prev (c07d78cc), but was
> 00001000. (next=d854236c).
> Modules linked in: sunrpc uinput virtio_net virtio_balloon i2c_piix4 pcspkr
> i2c_core virtio_blk virtio_pci virtio_ring virtio [last unloaded:
> scsi_wait_scan]
> Pid: 1394, comm: tee Not tainted 2.6.31-rc9 #51
> Call Trace:
>  [<c0438424>] warn_slowpath_common+0x65/0x7c
>  [<c05371b3>] ? __list_add+0x27/0x5c
>  [<c043846f>] warn_slowpath_fmt+0x24/0x27
>  [<c05371b3>] __list_add+0x27/0x5c
>  [<c047f050>] list_add+0xa/0xc
>  [<c047f8f5>] trace_add_event_call+0x60/0x97
>  [<c0483133>] command_trace_probe+0x42c/0x51b
>  [<c044a1b3>] ? remove_wait_queue+0x22/0x27
>  [<c042a9c0>] ? __wake_up+0x32/0x3b
>  [<c04832f6>] probes_write+0xd4/0x10a
>  [<c0483222>] ? probes_write+0x0/0x10a
>  [<c04b27a9>] vfs_write+0x80/0xdf
>  [<c04b289c>] sys_write+0x3b/0x5d
>  [<c0670d41>] syscall_call+0x7/0xb
> ---[ end trace 2b962b5dc1fdc07d ]---
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>

[PATCH tracing/kprobes 5/6] tracing/kprobes: Fix profiling alignment for perf_counter buffer

[Masami Hiramatsu]

Fix *probe_profile_func() to align buffer size, since pref_counter requires.

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Jim Keniston <jkenisto@us.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Frank Ch. Eigler <fche@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jason Baron <jbaron@redhat.com>
Cc: K.Prasad <prasad@linux.vnet.ibm.com>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tom Zanussi <tzanussi@gmail.com>
---

 kernel/trace/trace_kprobe.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 70b632c..d8db935 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1149,18 +1149,23 @@ static __kprobes int kprobe_profile_func(struct kprobe *kp,
 	struct trace_probe *tp = container_of(kp, struct trace_probe, rp.kp);
 	struct ftrace_event_call *call = &tp->call;
 	struct kprobe_trace_entry *entry;
-	int size, i, pc;
+	int size, __size, i, pc;
 	unsigned long irq_flags;
 
 	local_save_flags(irq_flags);
 	pc = preempt_count();
 
-	size = SIZEOF_KPROBE_TRACE_ENTRY(tp->nr_args);
+	__size = SIZEOF_KPROBE_TRACE_ENTRY(tp->nr_args);
+	size = ALIGN(__size + sizeof(u32), sizeof(u64));
+	size -= sizeof(u32);
 
 	do {
 		char raw_data[size];
 		struct trace_entry *ent;
-
+		/*
+		 * Zero dead bytes from alignment to avoid stack leak
+		 * to userspace
+		 */
 		*(u64 *)(&raw_data[size - sizeof(u64)]) = 0ULL;
 		entry = (struct kprobe_trace_entry *)raw_data;
 		ent = &entry->ent;
@@ -1183,13 +1188,15 @@ static __kprobes int kretprobe_profile_func(struct kretprobe_instance *ri,
 	struct trace_probe *tp = container_of(ri->rp, struct trace_probe, rp);
 	struct ftrace_event_call *call = &tp->call;
 	struct kretprobe_trace_entry *entry;
-	int size, i, pc;
+	int size, __size, i, pc;
 	unsigned long irq_flags;
 
 	local_save_flags(irq_flags);
 	pc = preempt_count();
 
-	size = SIZEOF_KRETPROBE_TRACE_ENTRY(tp->nr_args);
+	__size = SIZEOF_KRETPROBE_TRACE_ENTRY(tp->nr_args);
+	size = ALIGN(__size + sizeof(u32), sizeof(u64));
+	size -= sizeof(u32);
 
 	do {
 		char raw_data[size];


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH tracing/kprobes 0/6] tracing/kprobes: kprobe-based event tracer update and perf support

[Frederic Weisbecker]

On Mon, Sep 14, 2009 at 04:48:47PM -0400, Masami Hiramatsu wrote:
> Hi,
> 
> Here are bugfixes for kprobe-based event tracer. Thank you for review
> and reporting bugs! I fixed it. And also I decided to disable
> kprobe event when defining, since the events outputs may suddenly
> mess up the ftrace ring_buffer.
> 
> Frederic, could you see the patch 4/6? which is what I said on the
> previous threads.
> 
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (6):
>       tracing/kprobes: Disable kprobe events by default
>       tracing/kprobes: Fix profiling alignment for perf_counter buffer
>       tracing/kprobes: Add probe handler dispatcher for support perf and ftrace
>       ftrace: Fix trace_remove_event_call() to lock trace_event_mutex
>       ftrace: Fix trace_add_event_call() to initialize list
>       tracing/kprobes: Fix trace_probe registration order



Applied in

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	tracing/kprobes

I've included Ananth fix too.

This time it looks like I haven't lost the last patch.
But I should double-check each morning as I suspect I become
a sleepwalker who does random git-reset in midnight...