SMP races in proc with thread_struct

SMP races in proc with thread_struct

[Todd Inglett]

Perhaps this is old news...but...

I can easily create a race when reading /proc/<pid>/stat
(fs/proc/{base.c,array.c}) where a rapidly reading application, such as
"top", starts reading stats for a thread which goes away during the
read.  This is easily reproduced with a program that rapidly forks and
exits while top is running.

On inspection, I don't see how the code can expect the thread_struct to
stay around since it is not holding any lock for the duration of its
use.  The code could hold the thread_struct's lock (after verifying it
still exists while holding tasklist_lock I would imagine), but for
performance I would think a better solution would be to copy the struct
since stale data is probably ok in this case.

Dereferencing a non-existent thread_struct is clearly not ok.

Would anyone familiar with this code care to comment?
-- 
-todd

Re: SMP races in proc with thread_struct

[Alexander Viro]

On Tue, 1 May 2001, Todd Inglett wrote:

> Perhaps this is old news...but...
> 
> I can easily create a race when reading /proc/<pid>/stat
> (fs/proc/{base.c,array.c}) where a rapidly reading application, such as
> "top", starts reading stats for a thread which goes away during the
> read.  This is easily reproduced with a program that rapidly forks and
> exits while top is running.
> 
> On inspection, I don't see how the code can expect the thread_struct to
> stay around since it is not holding any lock for the duration of its
> use.  The code could hold the thread_struct's lock (after verifying it
> still exists while holding tasklist_lock I would imagine), but for
> performance I would think a better solution would be to copy the struct
> since stale data is probably ok in this case.
> 
> Dereferencing a non-existent thread_struct is clearly not ok.
> 
> Would anyone familiar with this code care to comment?

Code bumps the reference count of couple of pages that hold task_struct
when it opens the file.

Thus when task exits they won't be freed and won't be reused. Checking
that exit had already happened is trivial (and done).  And for anything
long said lock is held only while we acquire the reference
to task_struct component.

When you finally release the dentry of /proc/<pid> (and thus are guaranteed
that none of its children are used) you drop the reference to these two pages.
If process had exited while you were holding them - well, that's when
they are freed.

I really wonder what you've managed to reproduce - normal behaviour is
that as soon as task exits you can't access /proc/<pid> and any
access to already opened files in it fails with -ENOENT. Had you been
able to get something else?

Re: SMP races in proc with thread_struct

[Todd Inglett]

Alexander Viro wrote:
> 
> On Tue, 1 May 2001, Todd Inglett wrote:
> 
> > Perhaps this is old news...but...
> >
> > I can easily create a race when reading /proc/<pid>/stat
> > (fs/proc/{base.c,array.c}) where a rapidly reading application, such as
> > "top", starts reading stats for a thread which goes away during the
> > read.  This is easily reproduced with a program that rapidly forks and
> > exits while top is running.
> >
> > On inspection, I don't see how the code can expect the thread_struct to
> > stay around since it is not holding any lock for the duration of its
> > use.  The code could hold the thread_struct's lock (after verifying it
> > still exists while holding tasklist_lock I would imagine), but for
> > performance I would think a better solution would be to copy the struct
> > since stale data is probably ok in this case.
> >
> > Dereferencing a non-existent thread_struct is clearly not ok.
> >
> > Would anyone familiar with this code care to comment?
> 
> Code bumps the reference count of couple of pages that hold task_struct
> when it opens the file.

Yes that's right.  [Note that I erroneously wrote thread_struct when I
meant task_struct].

On closer inspection I see it is the *parent* task_struct that is the
problem here.  The task has indeed exited and the memory for the
task_struct is correctly held.  However, the /proc code will grab
tasklist_lock and dereference the parent task_struct to get the parent
pid.  Grabbing tasklist_lock is good, of course, when the task is live
because the parent could be switched at any time.  But in this case the
child is already done.  And so is the parent.  So the child's parent
task_struct is gone, but the stale held task_struct still points to
where it once was.

Does this sound like a possible scenerio?

-- 
-todd

Re: SMP races in proc with thread_struct

[Todd Inglett]

Ok, I've got this isolated.  Here's the sequence of events:

1.  Some process T (probably "top") opens /proc/N/stat.
2.  While holding tasklist_lock the proc code does a get_task_struct()
to add a ref count to the page.
3.  Process N exits.
4.  The parent of process N exits.
5.  Process T reads from the open file.  This calls proc_pid_stat()
which dereferences N's task_struct.  This is ok as Alexander points out
because a reference is held.
6.  Using N's task_struct process T attempt to dereference the *parent*
task struct.  It assumes this is ok because:
	A) it is holding tasklist_lock so N cannot be reparented in a race.
	B) every process *always* has a valid parent.

But this is where hell breaks loose.  Every process has a valid parent
-- unless it is dead and nobody cares.  Process N has already exited and
released from the tasklist while its parent was still alive.  There was
no reason to reparent it.  It just got released.  So N's task_struct has
a dangling ptr to its parent.  Nobody is holding the parent task_struct,
either.  When the parent died memory for its task_struct was released. 
This is ungood.


My opinion here is that this is proc's problem.  When we free a
task_struct it could be "cleaned up" of dangling ptrs, but this is a
hack to cover a bug in proc.

This is not isolated to the parent task_struct, either.  The task_struct
mm is also dereferenced.  It is pretty easy to validate a parent
task_struct ptr (just hold tasklist_lock and run the list to check if it
is still valid -- might not be the *right* task, but it will still be
valid).  However, how do you validate the mm is ok?

It would be nice if there were an easy way to detect when the process
gets into this state.  Certainly it is in this state if the page
reference count is 1.  But multiple processes *could* be reading the
same proc file holding additional artificial ref counts.

Hmm...perhaps if I scan the tasklist for my own task?  If I am not in
the list I either return an error or faked info.  I'll try this....

-- 
-todd

BTW, by adding code to validate the parent my test has now run for 18
hours on a 4-way without failure.  It otherwise would fail within the
first 5 minutes.

Re: SMP races in proc with thread_struct

[Keith Owens]

On Fri, 04 May 2001 07:34:20 -0500, 
Todd Inglett <tinglett@vnet.ibm.com> wrote:
>But this is where hell breaks loose.  Every process has a valid parent
>-- unless it is dead and nobody cares.  Process N has already exited and
>released from the tasklist while its parent was still alive.  There was
>no reason to reparent it.  It just got released.  So N's task_struct has
>a dangling ptr to its parent.  Nobody is holding the parent task_struct,
>either.  When the parent died memory for its task_struct was released. 
>This is ungood.

Wrap the reference to the parent task structure with exception table
recovery code, like copy_from_user().  If the dangling reference points
to valid memory the worst that will happen is that you read and report
gibberish for one output.  If the reference causes an exception then
recover and treat it as NULL.  For a read only case, the only important
thing is not to die, one occurrence of bad data is tolerable.

Re: SMP races in proc with thread_struct

[Andreas Schwab]

Keith Owens <kaos@ocs.com.au> writes:

|> On Fri, 04 May 2001 07:34:20 -0500, 
|> Todd Inglett <tinglett@vnet.ibm.com> wrote:
|> >But this is where hell breaks loose.  Every process has a valid parent
|> >-- unless it is dead and nobody cares.  Process N has already exited and
|> >released from the tasklist while its parent was still alive.  There was
|> >no reason to reparent it.  It just got released.  So N's task_struct has
|> >a dangling ptr to its parent.  Nobody is holding the parent task_struct,
|> >either.  When the parent died memory for its task_struct was released. 
|> >This is ungood.
|> 
|> Wrap the reference to the parent task structure with exception table
|> recovery code, like copy_from_user().

Exception tables only protect accesses to user virtual memory.  Kernel
memory references must always be valid in the first place.

Andreas.

-- 
Andreas Schwab                                  "And now for something
SuSE Labs                                        completely different."
Andreas.Schwab@suse.de
SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5

Re: SMP races in proc with thread_struct

[Brian Gerst]

Andreas Schwab wrote:
> 
> Keith Owens <kaos@ocs.com.au> writes:
> 
> |> On Fri, 04 May 2001 07:34:20 -0500,
> |> Todd Inglett <tinglett@vnet.ibm.com> wrote:
> |> >But this is where hell breaks loose.  Every process has a valid parent
> |> >-- unless it is dead and nobody cares.  Process N has already exited and
> |> >released from the tasklist while its parent was still alive.  There was
> |> >no reason to reparent it.  It just got released.  So N's task_struct has
> |> >a dangling ptr to its parent.  Nobody is holding the parent task_struct,
> |> >either.  When the parent died memory for its task_struct was released.
> |> >This is ungood.
> |>
> |> Wrap the reference to the parent task structure with exception table
> |> recovery code, like copy_from_user().
> 
> Exception tables only protect accesses to user virtual memory.  Kernel
> memory references must always be valid in the first place.
> 
> Andreas.

The virtual address being accessed is irrelevant.  It's the address of
the faulting instruction that determines what the kernel will do if it
can't deal with a page fault.  If the access was made from kernel mode
the exception handler (if there is one) always gets invoked, otherwise
it oopses.

--

				Brian Gerst

Re: SMP races in proc with thread_struct

[Keith Owens]

On 04 May 2001 15:11:37 +0200, 
Andreas Schwab <schwab@suse.de> wrote:
>Keith Owens <kaos@ocs.com.au> writes:
>|> Wrap the reference to the parent task structure with exception table
>|> recovery code, like copy_from_user().
>
>Exception tables only protect accesses to user virtual memory.  Kernel
>memory references must always be valid in the first place.

Wrong.  Exception tables say that if the kernel gets an exception
between labels A and B then branch to fixup label C.  See show_regs()
in arch/i386/kernel/process.c and wrmsr_eio() in arch/i386/kernel/msr.c
for examples which do not depend on user virtual memory.

Re: SMP races in proc with thread_struct

[Andreas Ferber]

Hi,

On Fri, May 04, 2001 at 10:46:43PM +1000, Keith Owens wrote:

> For a read only case, the only important
> thing is not to die, one occurrence of bad data is tolerable.

Strong NACK. The pages where the bad data comes from may in some cases
already be reclaimed for other data, probably something security
relevant, which should never ever be given even read access by an
unauthorized user. Even if this event may be a very rare case, one
single occurrence of this is one to much.

Andreas
-- 
>Ich nehm nicht mal Zucker in den den Kaffee.
Dafür nehm ich nichtmal Kaffee.
   (Peter Backof + Bernhard Schultz in detebe)

Re: SMP races in proc with thread_struct

[Todd Inglett]

Andreas Ferber wrote:
> 
> On Fri, May 04, 2001 at 10:46:43PM +1000, Keith Owens wrote:
> 
> > For a read only case, the only important
> > thing is not to die, one occurrence of bad data is tolerable.
> 
> Strong NACK. The pages where the bad data comes from may in some cases
> already be reclaimed for other data, probably something security
> relevant, which should never ever be given even read access by an
> unauthorized user. Even if this event may be a very rare case, one
> single occurrence of this is one to much.

Agreed.  Worse, it is not readonly.  The /proc code task_lock's the task
struct, thus writing to it.

I'll post a patch shortly once I've tested it.  Worse case only if the
task is exiting I sweep the tasklist looking for the parent to see if
the parent is still valid.  I am not verifying if it is the actual
parent (it might be a new task allocated at the same spot).  I could
just report 0 (or 1) for the parent for any process that is exiting, but
then you won't be able to see the ppid for zombies.  Or is there another
state I can look for?  What I really need is PF_EXITED :).

I am a little concerned also about mm, file, tty and sig fields. These
appear to be NULLed in do_exit(), but I haven't tracked down tty and sig
yet.
-- 
-todd

Re: SMP races in proc with thread_struct

[Alexander Viro]

On Fri, 4 May 2001, Todd Inglett wrote:

> Ok, I've got this isolated.  Here's the sequence of events:
> 
> 1.  Some process T (probably "top") opens /proc/N/stat.
> 2.  While holding tasklist_lock the proc code does a get_task_struct()
> to add a ref count to the page.
> 3.  Process N exits.
> 4.  The parent of process N exits.
> 5.  Process T reads from the open file.  This calls proc_pid_stat()
> which dereferences N's task_struct.  This is ok as Alexander points out
> because a reference is held.
> 6.  Using N's task_struct process T attempt to dereference the *parent*
> task struct.  It assumes this is ok because:

	Where?

> 	A) it is holding tasklist_lock so N cannot be reparented in a race.
> 	B) every process *always* has a valid parent.
> 
> But this is where hell breaks loose.  Every process has a valid parent
> -- unless it is dead and nobody cares.  Process N has already exited and
> released from the tasklist while its parent was still alive.  There was
> no reason to reparent it.  It just got released.  So N's task_struct has
> a dangling ptr to its parent.  Nobody is holding the parent task_struct,
> either.  When the parent died memory for its task_struct was released. 
> This is ungood.

	If N is dead all accesses should return -ENOENT. No matter what
happens with its parent.

> My opinion here is that this is proc's problem.  When we free a
> task_struct it could be "cleaned up" of dangling ptrs, but this is a
> hack to cover a bug in proc.
> 
> This is not isolated to the parent task_struct, either.  The task_struct
> mm is also dereferenced.  It is pretty easy to validate a parent
> task_struct ptr (just hold tasklist_lock and run the list to check if it
> is still valid -- might not be the *right* task, but it will still be
> valid).  However, how do you validate the mm is ok?

exit_mm() cleans task->mm. _Before_ the process dies. And it should do
that, for a lot of reasons. General principle: if you are doing
garbage-collection upon removal of the last reference - _remove_
that reference. Before you call destructor. Anything else is simply
asking for races.

Besides, all the exit_foo() can be done by a very alive kernel threads.
Suppose that exit_mm() didn't clean ->mm.  Well, here comes losetup(8).
It binds loop device to a file and starts a thread for handling requests.
Said thread is created by kernel_thread(9). Which is a wrapper for clone(2).
So far, so good, but that thread gets a VM of parent. I.e. losteup.
That is _not_ good. For one thing, it means full-blown MMU switch whenever
we switch to loop_thread. And that will cost you. Since loop_thread has
no business using the userland part of MMU state it calls exit_mm(9) (it
calls daemonize(9). which, in turn, calls exit_mm(9)).

That picture is typical for kernel threads. knfsd, drivers' helper threads,
you name it. And unlike the relatively tame case of loop_thread (there
we have serialization between loop_thread and parent that will not
let parent to exit before loop_thread will do up(&lo->lo_sem) which is
after the daemonize() call) in general we can very well have parent
dead and gone by the time when child calls exit_mm().

See the problem? Child is very much alive, it's the sole owner of pointer
to mm_struct and it calls exit_mm(). Leaving the pointer around is _not_
a good idea.

[PATCH][RFC] Re: SMP races in proc with thread_struct

[Alexander Viro]

	Linus, could you consider the patch below? As it is, access to
/proc/<pid>/status of dead process with dead parent is possible and
leads to access to freed memory. Besides, cd /proc/<pid> means
that even after <pid> is gone, readdir() _and_ lookup on /proc/<pid> work.
Patch makes sure that ->p_pptr is NULL once the process is gone (fixes
readdir/lookup stuff) and adds obvious couple of checks in array.c.
								Al

diff -urN S5-pre1/fs/proc/array.c S5-pre1-p_pptr/fs/proc/array.c
--- S5-pre1/fs/proc/array.c	Sat Apr 28 02:12:56 2001
+++ S5-pre1-p_pptr/fs/proc/array.c	Fri May  4 13:15:47 2001
@@ -157,7 +157,9 @@
 		"Uid:\t%d\t%d\t%d\t%d\n"
 		"Gid:\t%d\t%d\t%d\t%d\n",
 		get_task_state(p),
-		p->pid, p->p_opptr->pid, p->p_pptr->pid != p->p_opptr->pid ? p->p_pptr->pid : 0,
+		p->pid, p->p_opptr->pid,
+		p->p_pptr && p->p_pptr->pid != p->p_opptr->pid
+			? p->p_pptr->pid : 0,
 		p->uid, p->euid, p->suid, p->fsuid,
 		p->gid, p->egid, p->sgid, p->fsgid);
 	read_unlock(&tasklist_lock);	
@@ -339,7 +341,7 @@
 	nice = task->nice;
 
 	read_lock(&tasklist_lock);
-	ppid = task->p_opptr->pid;
+	ppid = task->p_pptr ? task->p_opptr->pid : 0;
 	read_unlock(&tasklist_lock);
 	res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %ld %ld %lu %lu %ld %lu %lu %lu %lu %lu \
diff -urN S5-pre1/kernel/exit.c S5-pre1-p_pptr/kernel/exit.c
--- S5-pre1/kernel/exit.c	Fri Feb 16 22:52:15 2001
+++ S5-pre1-p_pptr/kernel/exit.c	Fri May  4 13:18:33 2001
@@ -62,6 +62,9 @@
 		current->counter += p->counter;
 		if (current->counter >= MAX_COUNTER)
 			current->counter = MAX_COUNTER;
+		write_lock_irq(&tasklist_lock);
+		p->p_pptr = NULL;
+		write_unlock_irq(&tasklist_lock);
 		free_task_struct(p);
 	} else {
 		printk("task releasing itself\n");