Re: Deadlock on the mm->mmap_sem

[(no name)]

> On Thu, Sep 20, 2001 at 09:01:13AM +0100, David Howells wrote:
> > 
> > Andrea Arcangeli <[e-mail]> wrote:
> > > the process doesn't need to lock multiple mm_structs at the same time.
> > 
> > fork, ptrace, /proc/pid/mem, /proc/pid/maps
> >

David, coredump is the only difficult recursive user of mmap_sem.
ptrace & /proc/pid/mem double buffer into kernel buffers, fork just
doesn't lock the new mm_struct - it's new, noone can get a pointer to it
before it's linked into the various lists.

> for /proc/<pid>/maps this check takes care of it of course (or it could
> get unfair again: only when we're faulting on our vm we're allowed to go
> through):
> 
>         if (task == current)
>                 down_read_recursive(&mm->mmap_sem, &current->mm_recursor);
>         else
>                 down_read(&mm->mmap_sem);
> 
Andrea, my rewrite of proc_pid_read_maps fixes that without any ugly
recursive/nonrecursive tests.

Short summary of the possible fixes for the deadlock:

* A simple unfair mmap_sem (rw_lock like) is not possible.
* Copying the mm_struct is ugly.
* A fair, recursive mmap_sem (a task that already owns the mmap_sem can
acquire it again without deadlocking, all other cases are fair). That's
what Andrea proposes. (Andrea, is that correct?)
* moving the locking into each coredump handler. The main advantage is
that for some coredump handlers down_read is not enough - e.g.
elf_core_dump should call down_write to prevent concurrent expand_stack
calls, and acquire the pagetable_lock around some lines (right now it
walks the page tables without locking). I'll check the other coredump
handlers - during a quick check I couldn't find any oopsable races if
only a read lock is taken.

I'll write a patch that moves the locking into the coredump handlers,
then we can compare that with Andrea's proposal.

--
	Manfred