Re: [patch] fixed both processes in D state and the /proc/ oopses [Re: [patch] Fixed the race that was oopsing Linux-2.2.0]

["Stephen C. Tweedie" <sct@redhat.com>]

Hi,

On Wed, 27 Jan 1999 13:15:25 +0100 (CET), Andrea Arcangeli
<andrea@e-mind.com> said:

> - * Fixed a race between mmget() and mmput(): added the mm_lock spinlock in
> - * the task_struct to serialize accesses to the tsk->mm field.
> + * Fixed a race between mmget() and mmput(): added the mm_lock spinlock
> + * to serialize accesses to the tsk->mm field.

I don't buy it, because we've seen these on UP machines too.  Besides,
in all of the fork/exit/procfs code paths which look to be relevant to
the reported oopses, we already hold the global kernel lock by the time
we start fiddling with the mm references.  Adding yet another spinlock
should make no difference at all to the locking.

I think the real problem is in the new procfs code in fs/proc/array.c
calling mmget()/mmput() the way it does.  The get_mm_and_lock() function
tries to be safe, but in a couple of places we do not use it, instead
doing something like:

	get_status() {
		tsk = grab_task(pid);
		task_mem() {
			down(&mm->mmap_sem);
			/* Walk the vma tree */
			up(&mm->mmap_sem);
		}
		release_task(tsk);
	}

grab_task() includes

	if (tsk && tsk->mm && tsk->mm != &init_mm)
		mmget(tsk->mm);

and release_task does

	if (tsk != current && tsk->mm && tsk->mm != &init_mm)
		mmput(tsk->mm);

In other words, we are grabbing a task, pinning the mm_struct, blocking
for the mm semaphore and releasing the task's *current* mm_struct, which
is not necessarily the same as it was before we blocked.  In particular,
if we catch the process during a fork+exec, then it is perfectly
possible for tsk->mm to change here.

Note that this was not a problem in the original version of this patch
which just copied the task_struct in grab_task(), because that way we
would always be releasing the same mm that we grabbed.

Can anybody give a good reason why we need to block in array.c at all?
I don't see why we need the down/up(&mm->mmap_sem) code, since we should
already hold the global kernel lock and the vma tree should remain
intact while we inspect it as long as we do not drop that lock.  If we
do keep that lock intact from beginning to end then we cannot run the
risk of tsk->mm changing out from under our feet.

--Stephen
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/