kgdb segv in the latest tip due to perf ctx changes

kgdb segv in the latest tip due to perf ctx changes

[Jiri Olsa]

hi,

while starting kgdb early debug on latest tip tree,
I got SIGSEGV inside kernel in following location:

fetch_bp_busy_slots (bp=0xffff880075f69000) at
kernel/hw_breakpoint.c:137
137             struct task_struct *tsk = bp->ctx->task;


the segv is caused by the kgdb code assuming the event has
ctx field defined, which is not true at the the time
by the following backtrace:


(gdb) bt
#0  fetch_bp_busy_slots (bp=0xffff880075f69000) at kernel/hw_breakpoint.c:137
#1  __reserve_bp_slot (bp=0xffff880075f69000) at kernel/hw_breakpoint.c:314
#2  0xffffffff810ac0c5 in reserve_bp_slot (bp=0xffff880075f69000) at kernel/hw_breakpoint.c:336
#3  0xffffffff810ac0f6 in register_perf_hw_breakpoint (bp=0xffff880075f69000) at kernel/hw_breakpoint.c:412
#4  0xffffffff810ac165 in hw_breakpoint_event_init (bp=0xffff880075f69000) at kernel/hw_breakpoint.c:580
#5  0xffffffff810a3da6 in perf_init_event (event=0xffff880075f69000) at kernel/perf_event.c:5268
#6  0xffffffff810aa658 in perf_event_alloc (attr=0xffff88007899ddf0, cpu=0, group_leader=0xffff880075f69000, parent_event=0x0, 
    overflow_handler=0) at kernel/perf_event.c:5356
#7  0xffffffff810aae6c in perf_event_create_kernel_counter (attr=<value optimized out>, cpu=0, task=0x0, 
    overflow_handler=<value optimized out>) at kernel/perf_event.c:5752
#8  0xffffffff810ab8b5 in register_wide_hw_breakpoint (attr=0xffff88007899ddf0, triggered=0) at kernel/hw_breakpoint.c:518
#9  0xffffffff81025e2a in kgdb_arch_late () at arch/x86/kernel/kgdb.c:646
#10 0xffffffff8107be62 in kgdb_register_callbacks (new_dbg_io_ops=0xffffffff81a3de00) at kernel/debug/debug_core.c:802
#11 kgdb_register_io_module (new_dbg_io_ops=0xffffffff81a3de00) at kernel/debug/debug_core.c:906
#12 0xffffffff812662bc in configure_kgdboc () at drivers/serial/kgdboc.c:137
#13 0xffffffff81ae805b in init_kgdboc () at drivers/serial/kgdboc.c:159
#14 0xffffffff81abd61e in do_one_initcall (fn=0xffffffff81ae8047 <init_kgdboc>) at init/main.c:750
#15 0xffffffff81abd828 in do_initcalls (unused=<value optimized out>) at init/main.c:780
#16 do_basic_setup (unused=<value optimized out>) at init/main.c:801
#17 kernel_init (unused=<value optimized out>) at init/main.c:893
#18 0xffffffff810033d4 in ?? () at arch/x86/kernel/entry_64.S:1156
#19 0x0000000000000000 in ?? ()



I found out it's due to foolowing commit, that's causing the init code
to be called without the ctx field being defined...

commit c3f00c70276d8ae82578c8b773e2db657f69a478
Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date:   Wed Aug 18 14:37:15 2010 +0200


I'm not sure if kgdb is wrong assuming the filed exists or perf code
not defining the ctx field at that time..

I can reproduce easily, let me know if I can help

wbr,
jirka

Re: kgdb segv in the latest tip due to perf ctx changes

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

Jiri,

Can you try this simple patch which is attached?



On 09/24/2010 01:04 PM, Jiri Olsa wrote:
> while starting kgdb early debug on latest tip tree,
> I got SIGSEGV inside kernel in following location:
>
>   
[clip]
> I found out it's due to foolowing commit, that's causing the init code
> to be called without the ctx field being defined...
>
> commit c3f00c70276d8ae82578c8b773e2db657f69a478
> Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date:   Wed Aug 18 14:37:15 2010 +0200
>
>
>   

I took a look at the tip core, and the ctx parameter is no longer passed
into the perf_event_alloc() from perf_event_create_kernel_counter() kgdb
no longer gets it filled in for free.

The reality is that kgdb never had a true context or a way to mark the
hw breakpoint as a kernel only context for the hw breakpoint
reservations.  The patch is only a short term fix perhaps until on of
the perf guys explains the right way to use it. :-)

Thanks,
Jason.


[-- Attachment #2: 0001-Fix-null-dereference-when-using-early-kgdb.patch --]
[-- Type: text/x-diff, Size: 822 bytes --]

>From 17f3febd001a26aee9a75c61152b60b7e0ae1ea9 Mon Sep 17 00:00:00 2001
From: Jason Wessel <jason.wessel@windriver.com>
Date: Fri, 24 Sep 2010 15:21:11 -0500
Subject: [PATCH] Fix null dereference when using early kgdb

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/hw_breakpoint.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 3b714e8..3c7ccdf 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -213,7 +213,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
 	       int weight)
 {
 	int cpu = bp->cpu;
-	struct task_struct *tsk = bp->ctx->task;
+	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
 
 	/* Pinned counter cpu profiling */
 	if (!tsk) {
-- 
1.6.3.3

Re: kgdb segv in the latest tip due to perf ctx changes

[Peter Zijlstra]

On Fri, 2010-09-24 at 15:30 -0500, Jason Wessel wrote:
> Jiri,
> 
> Can you try this simple patch which is attached?
> 
> 
> 
> On 09/24/2010 01:04 PM, Jiri Olsa wrote:
> > while starting kgdb early debug on latest tip tree,
> > I got SIGSEGV inside kernel in following location:
> >
> >   
> [clip]
> > I found out it's due to foolowing commit, that's causing the init code
> > to be called without the ctx field being defined...
> >
> > commit c3f00c70276d8ae82578c8b773e2db657f69a478
> > Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Date:   Wed Aug 18 14:37:15 2010 +0200
> >
> >
> >   
> 
> I took a look at the tip core, and the ctx parameter is no longer passed
> into the perf_event_alloc() from perf_event_create_kernel_counter() kgdb
> no longer gets it filled in for free.
> 
> The reality is that kgdb never had a true context or a way to mark the
> hw breakpoint as a kernel only context for the hw breakpoint
> reservations.  The patch is only a short term fix perhaps until on of
> the perf guys explains the right way to use it. :-)

Argh, yes, its using the ctx rather early.. we cannot have a context
before we've initialized the event, and here it looks like hw_breakpoint
wants to use the context to initialize the event, chick, egg, etc..

Frederic, anything we can do about that?

> differences between files attachment
> (0001-Fix-null-dereference-when-using-early-kgdb.patch)
> From 17f3febd001a26aee9a75c61152b60b7e0ae1ea9 Mon Sep 17 00:00:00 2001
> From: Jason Wessel <jason.wessel@windriver.com>
> Date: Fri, 24 Sep 2010 15:21:11 -0500
> Subject: [PATCH] Fix null dereference when using early kgdb
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> ---
>  kernel/hw_breakpoint.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> index 3b714e8..3c7ccdf 100644
> --- a/kernel/hw_breakpoint.c
> +++ b/kernel/hw_breakpoint.c
> @@ -213,7 +213,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
>  	       int weight)
>  {
>  	int cpu = bp->cpu;
> -	struct task_struct *tsk = bp->ctx->task;
> +	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
>  
>  	/* Pinned counter cpu profiling */
>  	if (!tsk) {

That'll probably screw over some accounting, not sure what tsk is used
for there.

Re: kgdb segv in the latest tip due to perf ctx changes

[Frederic Weisbecker]

On Sat, Sep 25, 2010 at 02:29:20AM +0200, Peter Zijlstra wrote:
> On Fri, 2010-09-24 at 15:30 -0500, Jason Wessel wrote:
> > Jiri,
> > 
> > Can you try this simple patch which is attached?
> > 
> > 
> > 
> > On 09/24/2010 01:04 PM, Jiri Olsa wrote:
> > > while starting kgdb early debug on latest tip tree,
> > > I got SIGSEGV inside kernel in following location:
> > >
> > >   
> > [clip]
> > > I found out it's due to foolowing commit, that's causing the init code
> > > to be called without the ctx field being defined...
> > >
> > > commit c3f00c70276d8ae82578c8b773e2db657f69a478
> > > Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Date:   Wed Aug 18 14:37:15 2010 +0200
> > >
> > >
> > >   
> > 
> > I took a look at the tip core, and the ctx parameter is no longer passed
> > into the perf_event_alloc() from perf_event_create_kernel_counter() kgdb
> > no longer gets it filled in for free.
> > 
> > The reality is that kgdb never had a true context or a way to mark the
> > hw breakpoint as a kernel only context for the hw breakpoint
> > reservations.  The patch is only a short term fix perhaps until on of
> > the perf guys explains the right way to use it. :-)
> 
> Argh, yes, its using the ctx rather early.. we cannot have a context
> before we've initialized the event, and here it looks like hw_breakpoint
> wants to use the context to initialize the event, chick, egg, etc..
> 
> Frederic, anything we can do about that?



Jason's patch is partially good, it just lacks one place to handle.
Jiri, can you test that?

diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index d71a987..d727c58 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -134,7 +134,7 @@ fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
 		    enum bp_type_idx type)
 {
 	int cpu = bp->cpu;
-	struct task_struct *tsk = bp->ctx->task;
+	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
 
 	if (cpu >= 0) {
 		slots->pinned = per_cpu(nr_cpu_bp_pinned[type], cpu);
@@ -213,7 +213,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
 	       int weight)
 {
 	int cpu = bp->cpu;
-	struct task_struct *tsk = bp->ctx->task;
+	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
 
 	/* Pinned counter cpu profiling */
 	if (!tsk) {



 
> > differences between files attachment
> > (0001-Fix-null-dereference-when-using-early-kgdb.patch)
> > From 17f3febd001a26aee9a75c61152b60b7e0ae1ea9 Mon Sep 17 00:00:00 2001
> > From: Jason Wessel <jason.wessel@windriver.com>
> > Date: Fri, 24 Sep 2010 15:21:11 -0500
> > Subject: [PATCH] Fix null dereference when using early kgdb
> > 
> > Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> > ---
> >  kernel/hw_breakpoint.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> > index 3b714e8..3c7ccdf 100644
> > --- a/kernel/hw_breakpoint.c
> > +++ b/kernel/hw_breakpoint.c
> > @@ -213,7 +213,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
> >  	       int weight)
> >  {
> >  	int cpu = bp->cpu;
> > -	struct task_struct *tsk = bp->ctx->task;
> > +	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
> >  
> >  	/* Pinned counter cpu profiling */
> >  	if (!tsk) {
> 
> That'll probably screw over some accounting, not sure what tsk is used
> for there.


Nope it's ok. tsk is used to know if we are dealing with
a task/cpu bound breakpoint or a cpu wide bound one.

If tsk ends up being NULL, it will think it's a cpu wide bound
breakpoint, which it is in the case of kgdb breakpoints.

Re: kgdb segv in the latest tip due to perf ctx changes

[Jiri Olsa]

On Sat, Sep 25, 2010 at 03:55:35PM +0200, Frederic Weisbecker wrote:
> On Sat, Sep 25, 2010 at 02:29:20AM +0200, Peter Zijlstra wrote:
> > On Fri, 2010-09-24 at 15:30 -0500, Jason Wessel wrote:
> > > Jiri,
> > > 
> > > Can you try this simple patch which is attached?
> > > 
> > > 
> > > 
> > > On 09/24/2010 01:04 PM, Jiri Olsa wrote:
> > > > while starting kgdb early debug on latest tip tree,
> > > > I got SIGSEGV inside kernel in following location:
> > > >
> > > >   
> > > [clip]
> > > > I found out it's due to foolowing commit, that's causing the init code
> > > > to be called without the ctx field being defined...
> > > >
> > > > commit c3f00c70276d8ae82578c8b773e2db657f69a478
> > > > Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > > Date:   Wed Aug 18 14:37:15 2010 +0200
> > > >
> > > >
> > > >   
> > > 
> > > I took a look at the tip core, and the ctx parameter is no longer passed
> > > into the perf_event_alloc() from perf_event_create_kernel_counter() kgdb
> > > no longer gets it filled in for free.
> > > 
> > > The reality is that kgdb never had a true context or a way to mark the
> > > hw breakpoint as a kernel only context for the hw breakpoint
> > > reservations.  The patch is only a short term fix perhaps until on of
> > > the perf guys explains the right way to use it. :-)
> > 
> > Argh, yes, its using the ctx rather early.. we cannot have a context
> > before we've initialized the event, and here it looks like hw_breakpoint
> > wants to use the context to initialize the event, chick, egg, etc..
> > 
> > Frederic, anything we can do about that?
> 
> 
> 
> Jason's patch is partially good, it just lacks one place to handle.
> Jiri, can you test that?

it works ok

feel free to add
Tested-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: kgdb segv in the latest tip due to perf ctx changes

[Frederic Weisbecker]

On Mon, Sep 27, 2010 at 10:12:16AM +0200, Jiri Olsa wrote:
> On Sat, Sep 25, 2010 at 03:55:35PM +0200, Frederic Weisbecker wrote:
> > On Sat, Sep 25, 2010 at 02:29:20AM +0200, Peter Zijlstra wrote:
> > > On Fri, 2010-09-24 at 15:30 -0500, Jason Wessel wrote:
> > > > Jiri,
> > > > 
> > > > Can you try this simple patch which is attached?
> > > > 
> > > > 
> > > > 
> > > > On 09/24/2010 01:04 PM, Jiri Olsa wrote:
> > > > > while starting kgdb early debug on latest tip tree,
> > > > > I got SIGSEGV inside kernel in following location:
> > > > >
> > > > >   
> > > > [clip]
> > > > > I found out it's due to foolowing commit, that's causing the init code
> > > > > to be called without the ctx field being defined...
> > > > >
> > > > > commit c3f00c70276d8ae82578c8b773e2db657f69a478
> > > > > Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > > > Date:   Wed Aug 18 14:37:15 2010 +0200
> > > > >
> > > > >
> > > > >   
> > > > 
> > > > I took a look at the tip core, and the ctx parameter is no longer passed
> > > > into the perf_event_alloc() from perf_event_create_kernel_counter() kgdb
> > > > no longer gets it filled in for free.
> > > > 
> > > > The reality is that kgdb never had a true context or a way to mark the
> > > > hw breakpoint as a kernel only context for the hw breakpoint
> > > > reservations.  The patch is only a short term fix perhaps until on of
> > > > the perf guys explains the right way to use it. :-)
> > > 
> > > Argh, yes, its using the ctx rather early.. we cannot have a context
> > > before we've initialized the event, and here it looks like hw_breakpoint
> > > wants to use the context to initialize the event, chick, egg, etc..
> > > 
> > > Frederic, anything we can do about that?
> > 
> > 
> > 
> > Jason's patch is partially good, it just lacks one place to handle.
> > Jiri, can you test that?
> 
> it works ok
> 
> feel free to add
> Tested-by: Jiri Olsa <jolsa@redhat.com>
> 
> thanks,
> jirka


Actually I'm just realizing that, after the recent perf changes,
it does not only concern the early kgdb needs but every breakpoints
now that the context is found/allocated after the event gets initialized.

So I need to do a good bunch of changes in the breakpoints to fix that.

I'll come back with something.

Thanks.

Re: kgdb segv in the latest tip due to perf ctx changes

[Peter Zijlstra]

On Mon, 2010-09-27 at 17:47 +0200, Frederic Weisbecker wrote:
> 
> Actually I'm just realizing that, after the recent perf changes,
> it does not only concern the early kgdb needs but every breakpoints
> now that the context is found/allocated after the event gets initialized.
> 
> So I need to do a good bunch of changes in the breakpoints to fix that.
> 
> I'll come back with something. 

Right, disregard my email from a minute ago then ;-)

Re: kgdb segv in the latest tip due to perf ctx changes

[Peter Zijlstra]

On Sat, 2010-09-25 at 15:55 +0200, Frederic Weisbecker wrote:

> > Frederic, anything we can do about that?
> 
> 
> 
> Jason's patch is partially good, it just lacks one place to handle.
> Jiri, can you test that?
> 
> diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> index d71a987..d727c58 100644
> --- a/kernel/hw_breakpoint.c
> +++ b/kernel/hw_breakpoint.c
> @@ -134,7 +134,7 @@ fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
>  		    enum bp_type_idx type)
>  {
>  	int cpu = bp->cpu;
> -	struct task_struct *tsk = bp->ctx->task;
> +	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
>  
>  	if (cpu >= 0) {
>  		slots->pinned = per_cpu(nr_cpu_bp_pinned[type], cpu);
> @@ -213,7 +213,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
>  	       int weight)
>  {
>  	int cpu = bp->cpu;
> -	struct task_struct *tsk = bp->ctx->task;
> +	struct task_struct *tsk = bp->ctx ? bp->ctx->task : NULL;
>  
>  	/* Pinned counter cpu profiling */
>  	if (!tsk) {

That's identical to writing *tsk = NULL;

You seem to be missing the detail that perf_event->ctx will _always_ be
NULL during pmu::event_init()


> > That'll probably screw over some accounting, not sure what tsk is used
> > for there.
> 
> 
> Nope it's ok. tsk is used to know if we are dealing with
> a task/cpu bound breakpoint or a cpu wide bound one.
> 
> If tsk ends up being NULL, it will think it's a cpu wide bound
> breakpoint, which it is in the case of kgdb breakpoints.

See above, there's currently no way to know that in pmu::event_init().