Re: Jeste jeden bug

Re: Jeste jeden bug

[Jiri Kosina]

[ the original mail was sent by Zdenek to me offlist ]

On Wed, 13 Feb 2008, Zdenek Kabelac wrote:

> [   68.379022] gnome-settings-[2941] trap int3 ip:3d2c840f34
> sp:7fff36f5d100 error:0<3>BUG: sleeping function called from invalid
> context at kernel/rwsem.c:21
> [   68.379039] in_atomic():1, irqs_disabled():0
> [   68.379044] no locks held by gnome-settings-/2941.
> [   68.379050] Pid: 2941, comm: gnome-settings- Not tainted 2.6.25-rc1 #30
> [   68.379054]
> [   68.379056] Call Trace:
> [   68.379061]  <#DB>  [<ffffffff81064883>] ? __debug_show_held_locks+0x13/0x30
> [   68.379109]  [<ffffffff81036765>] __might_sleep+0xe5/0x110
> [   68.379123]  [<ffffffff812f2240>] down_read+0x20/0x70
> [   68.379137]  [<ffffffff8109cdca>] print_vma_addr+0x3a/0x110
> [   68.379152]  [<ffffffff8100f435>] do_trap+0xf5/0x170
> [   68.379168]  [<ffffffff8100f52b>] do_int3+0x7b/0xe0
> [   68.379180]  [<ffffffff812f4a6f>] int3+0x9f/0xd0
> [   68.379203]  <<EOE>>
> [   68.379229]  in libglib-2.0.so.0.1505.0[3d2c800000+dc000]

Andi,

in commit 03252919 you introduced print_vma_addr(), but this funcion for 
obvious reasons takes mmap_sem, so it is not safe to call it from atomic 
context (i.e. do_trap(), for example), which is behavior your patch 
introduced.

-- 
Jiri Kosina
SUSE Labs

print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Jiri Kosina]

[ sorry for forgetting to change subject properly :) ]

On Wed, 13 Feb 2008, Jiri Kosina wrote:

> [ the original mail was sent by Zdenek to me offlist ]
> 
> On Wed, 13 Feb 2008, Zdenek Kabelac wrote:
> 
> > [   68.379022] gnome-settings-[2941] trap int3 ip:3d2c840f34
> > sp:7fff36f5d100 error:0<3>BUG: sleeping function called from invalid
> > context at kernel/rwsem.c:21
> > [   68.379039] in_atomic():1, irqs_disabled():0
> > [   68.379044] no locks held by gnome-settings-/2941.
> > [   68.379050] Pid: 2941, comm: gnome-settings- Not tainted 2.6.25-rc1 #30
> > [   68.379054]
> > [   68.379056] Call Trace:
> > [   68.379061]  <#DB>  [<ffffffff81064883>] ? __debug_show_held_locks+0x13/0x30
> > [   68.379109]  [<ffffffff81036765>] __might_sleep+0xe5/0x110
> > [   68.379123]  [<ffffffff812f2240>] down_read+0x20/0x70
> > [   68.379137]  [<ffffffff8109cdca>] print_vma_addr+0x3a/0x110
> > [   68.379152]  [<ffffffff8100f435>] do_trap+0xf5/0x170
> > [   68.379168]  [<ffffffff8100f52b>] do_int3+0x7b/0xe0
> > [   68.379180]  [<ffffffff812f4a6f>] int3+0x9f/0xd0
> > [   68.379203]  <<EOE>>
> > [   68.379229]  in libglib-2.0.so.0.1505.0[3d2c800000+dc000]
> 
> Andi,
> 
> in commit 03252919 you introduced print_vma_addr(), but this funcion for 
> obvious reasons takes mmap_sem, so it is not safe to call it from atomic 
> context (i.e. do_trap(), for example), which is behavior your patch 
> introduced.
> 
> 

Re: print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Andi Kleen]

> > in commit 03252919 you introduced print_vma_addr(), but this funcion for 
> > obvious reasons takes mmap_sem, so it is not safe to call it from atomic 
> > context (i.e. do_trap(), for example), which is behavior your patch 
> > introduced.

Ah yes -- this behaviour of int3 do_trap was always a source of bugs. I remember
fixing such things in this area several times (last time in the RT kernel), but 
I keep forgetting it. Sorry.

The correct fix is to run the int3 and debug handlers on the process stack 
when the fault originated from user space. Then they can run preemptive
and it's ok to schedule for the lock too.

I'll fix that tomorrow.

-Andi

Re: print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> > > [   68.379054]
> > > [   68.379056] Call Trace:
> > > [   68.379061]  <#DB>  [<ffffffff81064883>] ? __debug_show_held_locks+0x13/0x30
> > > [   68.379109]  [<ffffffff81036765>] __might_sleep+0xe5/0x110
> > > [   68.379123]  [<ffffffff812f2240>] down_read+0x20/0x70
> > > [   68.379137]  [<ffffffff8109cdca>] print_vma_addr+0x3a/0x110
> > > [   68.379152]  [<ffffffff8100f435>] do_trap+0xf5/0x170
> > > [   68.379168]  [<ffffffff8100f52b>] do_int3+0x7b/0xe0
> > > [   68.379180]  [<ffffffff812f4a6f>] int3+0x9f/0xd0
> > > [   68.379203]  <<EOE>>
> > > [   68.379229]  in libglib-2.0.so.0.1505.0[3d2c800000+dc000]

ouch. Could you try the fix below? This makes print_vma_addr() more 
robust and should fix the immediate bug.

The longer-term fix will be to not run int3 exception handlers in a 
non-preemptible context (like 32-bit does) - but that will need more 
testing.

	Ingo

---------------->
Subject: x86: fix "BUG: sleeping function called from invalid context" in print_vma_addr()
From: Ingo Molnar <mingo@elte.hu>
Date: Wed Feb 13 20:21:06 CET 2008

Jiri Kosina reported the following deadlock scenario with
show_unhandled_signals enabled:

 [   68.379022] gnome-settings-[2941] trap int3 ip:3d2c840f34
 sp:7fff36f5d100 error:0<3>BUG: sleeping function called from invalid
 context at kernel/rwsem.c:21
 [   68.379039] in_atomic():1, irqs_disabled():0
 [   68.379044] no locks held by gnome-settings-/2941.
 [   68.379050] Pid: 2941, comm: gnome-settings- Not tainted 2.6.25-rc1 #30
 [   68.379054]
 [   68.379056] Call Trace:
 [   68.379061]  <#DB>  [<ffffffff81064883>] ? __debug_show_held_locks+0x13/0x30
 [   68.379109]  [<ffffffff81036765>] __might_sleep+0xe5/0x110
 [   68.379123]  [<ffffffff812f2240>] down_read+0x20/0x70
 [   68.379137]  [<ffffffff8109cdca>] print_vma_addr+0x3a/0x110
 [   68.379152]  [<ffffffff8100f435>] do_trap+0xf5/0x170
 [   68.379168]  [<ffffffff8100f52b>] do_int3+0x7b/0xe0
 [   68.379180]  [<ffffffff812f4a6f>] int3+0x9f/0xd0
 [   68.379203]  <<EOE>>
 [   68.379229]  in libglib-2.0.so.0.1505.0[3d2c800000+dc000]

and tracked it down to:

  commit 03252919b79891063cf99145612360efbdf9500b
  Author: Andi Kleen <ak@suse.de>
  Date:   Wed Jan 30 13:33:18 2008 +0100

      x86: print which shared library/executable faulted in segfault etc. messages

the problem is that we call down_read() from an atomic context.

Solve this by returning from print_vma_addr() if the preempt count is
elevated. Update preempt_conditional_sti / preempt_conditional_cli to
unconditionally lift the preempt count even on !CONFIG_PREEMPT.

Reported-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/traps_64.c |    4 ++--
 mm/memory.c                |    7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Index: linux-x86.q/arch/x86/kernel/traps_64.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/traps_64.c
+++ linux-x86.q/arch/x86/kernel/traps_64.c
@@ -84,7 +84,7 @@ static inline void conditional_sti(struc
 
 static inline void preempt_conditional_sti(struct pt_regs *regs)
 {
-	preempt_disable();
+	inc_preempt_count();
 	if (regs->flags & X86_EFLAGS_IF)
 		local_irq_enable();
 }
@@ -95,7 +95,7 @@ static inline void preempt_conditional_c
 		local_irq_disable();
 	/* Make sure to not schedule here because we could be running
 	   on an exception stack. */
-	preempt_enable_no_resched();
+	dec_preempt_count();
 }
 
 int kstack_depth_to_print = 12;
Index: linux-x86.q/mm/memory.c
===================================================================
--- linux-x86.q.orig/mm/memory.c
+++ linux-x86.q/mm/memory.c
@@ -2711,6 +2711,13 @@ void print_vma_addr(char *prefix, unsign
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 
+	/*
+	 * Do not print if we are in atomic
+	 * contexts (in exception stacks, etc.):
+	 */
+	if (preempt_count())
+		return;
+
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, ip);
 	if (vma && vma->vm_file) {

Re: print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Jiri Kosina]

On Wed, 13 Feb 2008, Ingo Molnar wrote:

> The longer-term fix will be to not run int3 exception handlers in a 
> non-preemptible context (like 32-bit does) - but that will need more 
> testing.

Yup, exactly.

> Jiri Kosina reported the following deadlock scenario with
> show_unhandled_signals enabled:

Actually the kernel error message was reported to me by Zdenek. Zdenek, 
could you please try the fix?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Zdenek Kabelac]

2008/2/13, Jiri Kosina <jkosina@suse.cz>:
> On Wed, 13 Feb 2008, Ingo Molnar wrote:
>
> > The longer-term fix will be to not run int3 exception handlers in a
> > non-preemptible context (like 32-bit does) - but that will need more
> > testing.
>
> Yup, exactly.
>
> > Jiri Kosina reported the following deadlock scenario with
> > show_unhandled_signals enabled:
>
> Actually the kernel error message was reported to me by Zdenek. Zdenek,
> could you please try the fix?


Ok - great I do not get this BUG log
So this has fixed 2 problems a 3 other remains :)

I'll open them tomorrow I guess :) at my two were fixed pretty quickly.

PS: Ingo did you have a chance to check my kernel IPI problem ?

Zdenek

Re: print_vma_addr possible deadlock (was Re: Jeste jeden bug)

[Andi Kleen]

On Wednesday 13 February 2008 20:35:38 Ingo Molnar wrote:

> +	/*
> +	 * Do not print if we are in atomic
> +	 * contexts (in exception stacks, etc.):
> +	 */
> +	if (preempt_count())
> +		return;

It should print the address even in this case; just not resolve the VMA
name.

-Andi