oops in proc_pid_stat() on task->real_parent?

oops in proc_pid_stat() on task->real_parent?

[Dave Hansen]

I got this OOPS while running a relatively intensive PostgreSQL load on
my dual-CPU PIII with 2.6.9, while simultaneously running top.  Sorry,
no serial console, but I do have a screenshot:

	http://sprucegoose.sr71.net/~dave/oops/dsc02631.jpg

$ addr2line -e vmlinux-2.6.9 c017920e
/home/dave/blackbird/fs/proc/array.c:359

>From the disassembly:

  c01791f7:       e8 d0 2b 15 00          call   c02cbdcc <_read_lock>
  c01791fc:       83 c4 08                add    $0x8,%esp
  c01791ff:       83 be a4 00 00 00 00    cmpl   $0x0,0xa4(%esi)
  c0179206:       74 18                   je     c0179220 <proc_pid_stat+0x20c>
  c0179208:       8b 86 ac 00 00 00       mov    0xac(%esi),%eax
->c017920e:       8b 80 a4 00 00 00       mov    0xa4(%eax),%eax
  c0179214:       89 44 24 48             mov    %eax,0x48(%esp)
  c0179218:       eb 0e                   jmp    c0179228 <proc_pid_stat+0x214>
  c017921a:       8d b6 00 00 00 00       lea    0x0(%esi),%esi

cmpl   $0x0,0xa4(%esi)                # check task->pid
je     c0179220 <proc_pid_stat+0x20c> 
mov    0xac(%esi),%eax                # load task->real_parent
mov    0xa4(%eax),%eax                # load real_parent->pid
mov    %eax,0x48(%esp)                # store into ppid

It sure looks like the culprit is the dereference of real_parent.

        read_lock(&tasklist_lock);
        ppid = task->pid ? task->real_parent->pid : 0;
        read_unlock(&tasklist_lock);

Since I have DEBUG_PAGEALLOC turned on, I have the feeling that,
although eax is an apparently valid address (d1a0ea90), that particular
address was actually unmapped because the task had been freed at the
time.  Is there a reference that needs to be taken on ->real_parent
somewhere?

Anyway, this was with 2.6.9, and I've been so far unable to reproduce it
on the same kernel.  Although I'm not running current -bk, it looks like
there's been some churn in the same area since 2.6.9, so maybe it was
fixed.  

Thought I'd report it, just in case someone else sees the same thing.

-- Dave

Re: oops in proc_pid_stat() on task->real_parent?

[Andrew Morton]

Dave Hansen <dave@sr71.net> wrote:
>
> I got this OOPS while running a relatively intensive PostgreSQL load on
>  my dual-CPU PIII with 2.6.9, while simultaneously running top.  Sorry,
>  no serial console, but I do have a screenshot:
> 
>  	http://sprucegoose.sr71.net/~dave/oops/dsc02631.jpg
> 
>  $ addr2line -e vmlinux-2.6.9 c017920e

yup, we fixed that one.


From: Manfred Spraul <manfred@colorfullife.com>

proc_pid_status dereferences pointers in the task structure even if the
task is already dead.  This is probably the reason for the oops described
in

http://bugme.osdl.org/show_bug.cgi?id=3812

The attached patch removes the pointer dereferences by using pid_alive()
for testing that the task structure contents is still valid before
dereferencing them.  The task structure itself is guaranteed to be valid -
we hold a reference count.

Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/fs/proc/array.c       |    4 ++--
 25-akpm/fs/proc/base.c        |    5 -----
 25-akpm/include/linux/sched.h |   13 +++++++++++++
 3 files changed, 15 insertions(+), 7 deletions(-)

diff -puN fs/proc/array.c~use-pid_alive-in-proc_pid_status fs/proc/array.c
--- 25/fs/proc/array.c~use-pid_alive-in-proc_pid_status	2004-12-02 23:31:57.000000000 -0800
+++ 25-akpm/fs/proc/array.c	2004-12-02 23:31:57.000000000 -0800
@@ -171,8 +171,8 @@ static inline char * task_state(struct t
 		get_task_state(p),
 		(p->sleep_avg/1024)*100/(1020000000/1024),
 	       	p->tgid,
-		p->pid, p->pid ? p->group_leader->real_parent->tgid : 0,
-		p->pid && p->ptrace ? p->parent->pid : 0,
+		p->pid, pid_alive(p) ? p->group_leader->real_parent->tgid : 0,
+		pid_alive(p) && p->ptrace ? p->parent->pid : 0,
 		p->uid, p->euid, p->suid, p->fsuid,
 		p->gid, p->egid, p->sgid, p->fsgid);
 	read_unlock(&tasklist_lock);
diff -puN fs/proc/base.c~use-pid_alive-in-proc_pid_status fs/proc/base.c
--- 25/fs/proc/base.c~use-pid_alive-in-proc_pid_status	2004-12-02 23:31:57.000000000 -0800
+++ 25-akpm/fs/proc/base.c	2004-12-02 23:31:57.000000000 -0800
@@ -780,11 +780,6 @@ static struct inode_operations proc_pid_
 	.follow_link	= proc_pid_follow_link
 };
 
-static inline int pid_alive(struct task_struct *p)
-{
-	return p->pids[PIDTYPE_PID].nr != 0;
-}
-
 #define NUMBUF 10
 
 static int proc_readfd(struct file * filp, void * dirent, filldir_t filldir)
diff -puN include/linux/sched.h~use-pid_alive-in-proc_pid_status include/linux/sched.h
--- 25/include/linux/sched.h~use-pid_alive-in-proc_pid_status	2004-12-02 23:31:57.000000000 -0800
+++ 25-akpm/include/linux/sched.h	2004-12-02 23:31:57.000000000 -0800
@@ -671,6 +671,19 @@ static inline pid_t process_group(struct
 	return tsk->signal->pgrp;
 }
 
+/**
+ * pid_alive - check that a task structure is not stale
+ * @p: Task structure to be checked.
+ *
+ * Test if a process is not yet dead (at most zombie state)
+ * If pid_alive fails, then pointers within the task structure
+ * can be stale and must not be dereferenced.
+ */
+static inline int pid_alive(struct task_struct *p)
+{
+	return p->pids[PIDTYPE_PID].nr != 0;
+}
+
 extern void free_task(struct task_struct *tsk);
 extern void __put_task_struct(struct task_struct *tsk);
 #define get_task_struct(tsk) do { atomic_inc(&(tsk)->usage); } while(0)
_

Re: oops in proc_pid_stat() on task->real_parent?

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> yup, we fixed that one.

I thought the same thing, but this oops is from proc_pid_stat, not
proc_pid_status.  The code is now in do_task_stat(), and the oops is
within the orignal tasklist lock (instead of dropping and reaquiring the
lock).  So, might be fixed, but if so, I think for a different reason.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: oops in proc_pid_stat() on task->real_parent?

[Andrew Morton]

Chris Wright <chrisw@osdl.org> wrote:
>
> * Andrew Morton (akpm@osdl.org) wrote:
> > yup, we fixed that one.
> 
> I thought the same thing, but this oops is from proc_pid_stat, not
> proc_pid_status.  The code is now in do_task_stat(), and the oops is
> within the orignal tasklist lock (instead of dropping and reaquiring the
> lock).  So, might be fixed, but if so, I think for a different reason.
> 

Ah, thanks.

I'm not sure that the holding of tasklist_lock is going to save us there. 
But then, Manfred recently did an audit, so I'm probably missing something.

Manfred, should we do this?

--- 25/fs/proc/array.c~do_task_stat-use-pid_alive	2004-12-07 22:17:01.378528576 -0800
+++ 25-akpm/fs/proc/array.c	2004-12-07 22:17:10.140196600 -0800
@@ -370,7 +370,7 @@ static int do_task_stat(struct task_stru
 			stime += task->signal->stime;
 		}
 	}
-	ppid = task->pid ? task->group_leader->real_parent->tgid : 0;
+	ppid = pid_alive(task) ? task->group_leader->real_parent->tgid : 0;
 	read_unlock(&tasklist_lock);
 
 	if (!whole || num_threads<2)
_

Re: oops in proc_pid_stat() on task->real_parent?

[Dinakar Guniguntala]

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

On Tue, Dec 07, 2004 at 10:07:55PM -0800, Chris Wright wrote:
> * Andrew Morton (akpm@osdl.org) wrote:
> > yup, we fixed that one.
> 
> I thought the same thing, but this oops is from proc_pid_stat, not
> proc_pid_status.  The code is now in do_task_stat(), and the oops is
> within the orignal tasklist lock (instead of dropping and reaquiring the
> lock).  So, might be fixed, but if so, I think for a different reason.
> 
> thanks,
> -chris

hmmm they were two places that I had changed to reflect the parent.

This should fix it?

Signed-off-by: Dinakar Guniguntala <dino@in.ibm.com>



[-- Attachment #2: ps_stat-2.patch --]
[-- Type: text/plain, Size: 536 bytes --]

diff -Naurp linux-2.6.10-rc2.orig/fs/proc/array.c linux-2.6.10-rc2/fs/proc/array.c
--- linux-2.6.10-rc2.orig/fs/proc/array.c	2004-11-15 06:57:52.000000000 +0530
+++ linux-2.6.10-rc2/fs/proc/array.c	2004-12-08 12:54:40.000000000 +0530
@@ -370,7 +370,7 @@ static int do_task_stat(struct task_stru
 			stime += task->signal->stime;
 		}
 	}
-	ppid = task->pid ? task->group_leader->real_parent->tgid : 0;
+	ppid = pid_alive(task) ? task->group_leader->real_parent->tgid : 0;
 	read_unlock(&tasklist_lock);
 
 	if (!whole || num_threads<2)

Re: oops in proc_pid_stat() on task->real_parent?

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Chris Wright <chrisw@osdl.org> wrote:
> >
> > * Andrew Morton (akpm@osdl.org) wrote:
> > > yup, we fixed that one.
> > 
> > I thought the same thing, but this oops is from proc_pid_stat, not
> > proc_pid_status.  The code is now in do_task_stat(), and the oops is
> > within the orignal tasklist lock (instead of dropping and reaquiring the
> > lock).  So, might be fixed, but if so, I think for a different reason.
> > 
> 
> Ah, thanks.
> 
> I'm not sure that the holding of tasklist_lock is going to save us there. 
> But then, Manfred recently did an audit, so I'm probably missing something.
> 
> Manfred, should we do this?

Yeah, I wondered the same.  Although I don't see why pid_alive() check
would be useful if it's the real_parent that's gone.  Dave mentioned
that he's got slab poisoning enabled, and the real_parent pointer was
valid (i.e. not 6b6b6b6b).  So wouldn't tasklist_lock serialize against
exiting real_parent?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net