kernel 2.6.9-rc1-mm4 oops

Hi there,

This morning one of our (MySQL-)database serves crashed with the following kernel trace. Anyone has an idea what could've caused it? The machine is an SMP Xeon 2.8Ghz with 4G internal Reg. ECC ram running 4 scsi disks in sw raid 5 on a Debian (almost sid-)distribution.

The trace:

------------[ cut here ]------------
kernel BUG at kernel/exit.c:852!
invalid operand: 0000 [#1]
SMP
Modules linked in: ip_vs_wlc af_packet ipt_MARK iptable_mangle ip_tables ip_vs tg3 e1000 e100 eepro100 mii
nfsd exportfs nfs lockd sunrpc unix
CPU:    0
EIP:    0060:[<c011df03>]    Not tainted VLI
EFLAGS: 00010246   (2.6.9-rc1-mm4-fw-xeon.1)

EIP is at next_thread+0xc/0x41
eax: 00000000 ebx: 00000001 ecx: 00000001 edx: e93c3aa0
esi: 00000000 edi: e93c3aa0 ebp: 00000000 esp: f3893dd8
ds: 007b es: 007b ss: 0068
Process snmpd (pid: 1182, threadinfo=f3892000 task=f3fa1550)

Stack: c0182368 f3893f14 e93c3aa0 c016cecb c30c8a00 c011542b c03bfbe0 c30c8a00
       c017fcf6 e18d6eb0 e93c3aa0 0000000d c017fdad e93c3aa0 4143bbb4 247966f0
       c016c653 c03bfbe0 e18d6eb0 c03a4bc5 c01802a0 f3e56c20 e18d6eb0 0000000d
Call Trace:
 [<c0182368>] do_task_stat+0x279/0x752
 [<c016cecb>] alloc_inode+0x1b/0x146
 [<c011542b>] do_page_fault+0x19d/0x5c7
 [<c017fcf6>] task_dumpable+0x39/0x4a
 [<c017fdad>] proc_pid_make_inode+0xa6/0xe5
 [<c016c653>] d_rehash+0x55/0x79
 [<c01802a0>] proc_pident_lookup+0x100/0x26c
 [<c0161586>] real_lookup+0xcd/0xf0
 [<c016b468>] dput+0x24/0x209
 [<c0162247>] link_path_walk+0xa3e/0xd89
 [<c0182883>] proc_tgid_stat+0x1f/0x23
 [<c017f3ed>] proc_info_read+0x6a/0x9f
 [<c015417f>] vfs_read+0xbc/0x127
 [<c015444d>] sys_read+0x51/0x80
 [<c0105cdf>] syscall_call+0x7/0xb
Code: 8b 44 24 0c 89 04 24 e8 1d fc ff ff 83 ec 04 0f b6 44 24 08 c1 e0 08 89 04 24 e8 0a fc ff ff 89 c2

8b 80 d0 04 00 00 85 c0 75 08 <0f> 0b 54 03 e5

kernel/exit.c renamed from kernel/exit.c.nt +0 -0

@@ -848,10 +848,7 @@ asmlinkage long sys_exit(int error_code)
 task_t fastcall *next_thread(const task_t *p)

This patch removes sighand checks from the next_thread(), since they are incorrect and has nothing to do with the next_thread() function. So they could trigger BUG() when there were no actually bug at all.

Quoted 3 lines · Kirill Korotaev

This patch removes sighand checks from the next_thread(), since they are incorrect and has nothing to do with the next_thread() function. So they could trigger BUG() when there were no actually bug at all.

the problem is, generally it is not valid to have a thread on the thread list that has no ->sighand structure. This is what happens when we exit a task:

        write_lock_irq(&tasklist_lock);
	...
        __exit_sighand(p);
        __unhash_process(p);

the BUG() is useful for all the code that uses next_thread() - you can only do a safe next_thread() iteration if you've locked ->sighand.

so i believe your fix papers over the real bug which is the use of an almost-dead task for thread iterations. Since we've already done __unhash_process() not doing the BUG introduces a more subtle bug: the use of the stale PID pointers! So i believe the right fix is the one below, which (under the safety of read_lock(tasklock)) checks for the availability of task->sighand - and skips the thread iterations if so.

Quoted 2 lines

the BUG() is useful for all the code that uses next_thread() - you can only do a safe next_thread() iteration if you've locked ->sighand.

1. I don't see spin_lock() on p->sighand->siglock in do_task_stat() before calling next_thread(). And the check inside next_thread() permits only one of the locks to be taken:

        if (!spin_is_locked(&p->sighand->siglock) &&
                                !rwlock_is_locked(&tasklist_lock))

which is probably wrong, since tasklist_lock is always required!

2. I think the idea of checking sighand is quite obscure. Probably it would be better to call pid_alive() for check at such places in proc, isn't it?

3. And yes, now I agree that this check and BUG() prevented next_thread() from walking through the deleted list_head in tsk->pid_list.

But I would propose to reorganize these checks in next_thread() to something like this:

if (!rwlock_is_locked(&tasklist_lock) || p->pids[PIDTYPE_TGID].nr == 0)

	BUG();

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

4. If we do checks this way, then we can found strange proc numeric results. Suppose, you have read the do_task_stat() and it iterated through the threads and summed the times in this loop with next_thread(). Next, the thread dies and you can read the results w/o this loop and threads times, only this thread stats. Looks a bit invalid. Don't you think so? Maybe we should return an error?

Quoted 7 lines · Kirill Korotaev

Quoted 2 lines

the BUG() is useful for all the code that uses next_thread() - you can only do a safe next_thread() iteration if you've locked ->sighand.

1. I don't see spin_lock() on p->sighand->siglock in do_task_stat() before calling next_thread(). And the check inside next_thread() permits only one of the locks to be taken:

        if (!spin_is_locked(&p->sighand->siglock) &&
                                !rwlock_is_locked(&tasklist_lock))

Quoted 2 lines

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

yep - please send a new patch to Andrew.

Quoted 4 lines

Quoted 2 lines

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

yep - please send a new patch to Andrew.

I'll be awaiting the patch and give it a shot.

Thanks all for the feedback.

Quoted 5 lines · Roel van der Made

Quoted 3 lines

Quoted 2 lines

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

yep - please send a new patch to Andrew.

I'll be awaiting the patch and give it a shot.
Thanks all for the feedback.

you can try my patch too, it will do the job of fixing the bug. The other changes we talked about are only improvements to the debugging infrastructure.

Ingo

Quoted 9 lines · Ingo Molnar

Quoted 6 lines · Roel van der Made

Quoted 3 lines

Quoted 1 line

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

yep - please send a new patch to Andrew.

I'll be awaiting the patch and give it a shot.
Thanks all for the feedback.

Quoted 3 lines

you can try my patch too, it will do the job of fixing the bug. The other changes we talked about are only improvements to the debugging infrastructure.

Saw there's an mm5 release already, is your patch included in this one also?

Quoted 5 lines · Kirill Korotaev

Quoted 2 lines

the BUG() is useful for all the code that uses next_thread() - you can only do a safe next_thread() iteration if you've locked ->sighand.

1. I don't see spin_lock() on p->sighand->siglock in do_task_stat() before calling next_thread(). And the check inside next_thread() permits only one of the locks to be taken:

Quoted 4 lines

       if (!spin_is_locked(&p->sighand->siglock) &&
                               !rwlock_is_locked(&tasklist_lock))

which is probably wrong, since tasklist_lock is always required!

It's not 'wrong' in terms of correctness it's simply too restrictive for no reason. I agree that we should check for the tasklist lock only.

Quoted 3 lines

2. I think the idea of checking sighand is quite obscure. Probably it would be better to call pid_alive() for check at such places in proc, isn't it?

yeah, it's just as good of a check.

Quoted 7 lines

But I would propose to reorganize these checks in next_thread() to something like this:

if (!rwlock_is_locked(&tasklist_lock) || p->pids[PIDTYPE_TGID].nr == 0)

	BUG();

the last check ensures that we are still hashed and this check is more straithforward for understanding, agree?

yep - please send a new patch to Andrew.