Re: Deadlock on the mm->mmap_sem

[Andrea Arcangeli <andrea@suse.de>]

On Tue, Sep 18, 2001 at 09:31:40AM +0200, Manfred Spraul wrote:
> From: "Andrea Arcangeli" <andrea@suse.de>
> > > The mmap semaphore is a read-write semaphore, and it _is_
> permissible to
> > > call "copy_to_user()" and friends while holding the read lock.
> > >
> > > The bug appears to be in the implementation of the write semaphore -
> > > down_write() doesn't undestand that blocked writes must not block
> new
> > > readers, exactly because of this situation.
> >
> > Exactly, same reason for which we need the same property from the rw
> > spinlocks (to be allowed to read_lock without clearing irqs). Thanks
> so
> > much for reminding me about this! Unfortunately my rwsemaphores are
> > blocking readers at the first down_write (for the better fairness
> > property issuse, but I obviously forgotten that doing so I would
> > introduce such a deadlock).
> 
> i386 has a fair rwsemaphore, too - probably other archs must be modified
> as well.

yes, actually my patch was against the rwsem patch in -aa, and in -aa
I'm using the generic semaphores for all archs in the tree so it fixes
the race for all them. The mainline semaphores are slightly different.

> > The fix is a few liner for my
> > implementation, here it is:
> >
> 
> Obivously your patch fixes the race, but we could starve down_write() if
> there are many page faults.

Yes.

> IMHO modifying proc_pid_read_maps() is far simpler - I'm not aware of
> another recursive mmap_sem user.

if that's the very only place that could be a viable option but OTOH I
like to be allowed to use recursion on the read locks as with the
spinlocks. I think another option would be to have reacursion allowed on
the default read locks and then make a down_read_fair that will block at
if there's a down_write under us. we can very cleanly implement this,
the same can be done cleanly also for the spinlocks: read_lock_fair. One
can even mix the read_lock/read_lock_fair or the
down_read/down_read_fair together. For example assuming we use the
recursive semaphore fix in proc_pid_read_maps the down_read over there
could be converted to a down_read_fair (but that's just an exercise, if
the page fault isn't fair it doesn't worth to have proc_pid_read_maps
fair either).

Andrea