[PATCH] use pid_alive in proc_pid_status

[PATCH] use pid_alive in proc_pid_status

[Manfred Spraul]

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

Hi,

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.

What do you think? Are you aware of further instances where p->pid is 
still used to check if a thread is alive?

Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>


[-- Attachment #2: patch-pid-alive --]
[-- Type: text/plain, Size: 2082 bytes --]

// $Header$
// Kernel Version:
//  VERSION = 2
//  PATCHLEVEL = 6
//  SUBLEVEL = 10
//  EXTRAVERSION =-rc2
--- 2.6/include/linux/pid.h	2004-10-23 09:58:17.000000000 +0200
+++ build-2.6/include/linux/pid.h	2004-11-28 12:07:55.514992845 +0100
@@ -52,4 +52,6 @@
 			hlist_unhashed(&(task)->pids[type].pid_chain));	\
 	}								\
 
+extern int pid_alive(struct task_struct *p);
+
 #endif /* _LINUX_PID_H */
--- 2.6/kernel/pid.c	2004-11-19 18:54:37.000000000 +0100
+++ build-2.6/kernel/pid.c	2004-11-28 12:09:07.464302391 +0100
@@ -247,6 +247,19 @@
 	attach_pid(leader, PIDTYPE_SID, leader->signal->session);
 }
 
+/**
+ * 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.
+ */
+int pid_alive(struct task_struct *p)
+{
+	return p->pids[PIDTYPE_PID].nr != 0;
+}
+
 /*
  * The pid hash table is scaled according to the amount of memory in the
  * machine.  From a minimum of 16 slots up to 4096 slots at one gigabyte or
--- 2.6/fs/proc/base.c	2004-11-19 18:54:34.000000000 +0100
+++ build-2.6/fs/proc/base.c	2004-11-28 12:06:49.259448232 +0100
@@ -780,11 +780,6 @@
 	.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)
--- 2.6/fs/proc/array.c	2004-11-19 18:54:34.000000000 +0100
+++ build-2.6/fs/proc/array.c	2004-11-28 12:00:17.944726203 +0100
@@ -171,8 +171,8 @@
 		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);

Re: [PATCH] use pid_alive in proc_pid_status

[Linus Torvalds]

On Sun, 28 Nov 2004, Manfred Spraul wrote:
> 
> What do you think? Are you aware of further instances where p->pid is 
> still used to check if a thread is alive?

Looks good, except I hate how you have a function that does a single 
pointer derefence and a test.

There are cases where inline functions bloat up the code, but there are 
cases where a function call is bigger than the function body. This seems 
to be one of them.

		Linus

Re: [PATCH] use pid_alive in proc_pid_status

[Andrew Morton]

Manfred Spraul <manfred@colorfullife.com> wrote:
>
> +/**
>  + * 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.
>  + */
>  +int pid_alive(struct task_struct *p)
>  +{
>  +	return p->pids[PIDTYPE_PID].nr != 0;
>  +}

Can we not simply test p->exit_state?  That's already done in quite a few
places and making things consistent would be nice.

Re: [PATCH] use pid_alive in proc_pid_status

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> >  +int pid_alive(struct task_struct *p)
> >  +{
> >  +	return p->pids[PIDTYPE_PID].nr != 0;
> >  +}
> 
> Can we not simply test p->exit_state?  That's already done in quite a
> few places and making things consistent would be nice.

as long as it's accessed from under the tasklist_lock, it ought to be
fine to check for p->exit_state != EXIT_DEAD and dereference
p->group_leader afterwards.

	Ingo

Re: [PATCH] use pid_alive in proc_pid_status

[Manfred Spraul]

Ingo Molnar wrote:

>* Andrew Morton <akpm@osdl.org> wrote:
>
>  
>
>>> +int pid_alive(struct task_struct *p)
>>> +{
>>> +	return p->pids[PIDTYPE_PID].nr != 0;
>>> +}
>>>      
>>>
>>Can we not simply test p->exit_state?  That's already done in quite a
>>few places and making things consistent would be nice.
>>    
>>
>
>as long as it's accessed from under the tasklist_lock, it ought to be
>fine to check for p->exit_state != EXIT_DEAD and dereference
>p->group_leader afterwards.
>
>  
>
The tricky part is proc_pid_unhash()/proc_pid_flush(): Right now 
removing a pid from the pid bitmap and disabling /proc/<pid>/* is 
atomic: Both operations are done under tasklist_lock.
I think it would be better to modify pid_alive to p->exit_state and 
disable /proc/<pid>/* access when the exit state is set to DEAD, but 
that that would be a larger change. Probably unhash and flush could be 
merged into one function.
But I don't understand the lines in wait_task_zombie that reset 
exit_state from DEAD to ZOMBIE, so perhaps I overlook something.

--
    Manfred

Re: [PATCH] use pid_alive in proc_pid_status

[Roland McGrath]

> But I don't understand the lines in wait_task_zombie that reset 
> exit_state from DEAD to ZOMBIE, so perhaps I overlook something.

This happens when a sys_wait* call tries to reap a process, but then has
some problem like EFAULT.  It then abandons the reaping attempt by turning
DEAD back into ZOMBIE, so another sys_wait* call can succeed later.  So,
during this brief window it can be in DEAD though it in fact is never
reaped and the PID remains bound to that task_struct throughout.
If you don't want to rule out ZOMBIE, you can't really rule out DEAD.


Thanks,
Roland