locking question: do_mmap(), do_munmap()

which semaphores/spinlocks protect do_mmap() and do_munmap()?

do_mmap():
I wrote a test patch, and I found out that some (all?) callers call
lock_kernel(), but that mm->mmap_sem is _NOT ACQUIRED_. Eg sys_uselib()-> do_load_elf_??() -> do_mmap().

do_munmap():
???? I think here is a race:
sys_munmap() doesn't call lock_kernel(), it only acquires mm->mmap_sem. do_mmap() internally calls do_munmap(), ie with the kernel lock, but without mm->mmap_sem.

What about adding debugging runtime checks to these function? ie #defines which call down_trylock() and spin_trylock() and oops on missing locks?
We could define them to "(void)0" before 2.4.

--
Manfred
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

do_munmap() doesn't need the big lock. do_mmap() callers should grab the semaphore, unless they are sure that nobody else owns the same context. Big lock on do_mmap() is still needed. do_mmap() in do_load_elf_binary() is OK (we are the sole owners of context), but in the do_load_elf_library() it is _not_. Moreover, sys_uselib() may do interesting things to cloned processes. IMO the right thing would be to check for the number of mm users.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

I don't know the details of the mm implementation, but if there is only one user, then down(&mm->mmap_sem) will never sleep, and you loose nothing by getting the semaphore.

I would prefer a clean implementation, ie always down() before do_mmap(), and ASSERT_DOWN() macros to enforce this.

--
Manfred
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Yes (especially since sys_uselib() is not _too_ time-critical), but consider what will happen if the thread A does sys_uselib() while the thread B runs in the affected area. It's a different problem. As for the details - check where the new users may come from.

I don't think that it calls for ASSERT-style macros, though. IMO it's a matter of one big grep. As of 2.3.20-pre2 affected places are arch/mips/kernel/sysirix.c arch/sparc64/kernel/binfmt_aout32.c
fs/binfmt_aout.c
fs/binfmt_elf.c
drivers/char/drm/bufs.c (WTF is that?) drivers/sgi/char/{graphics.c,shmiq.c}

So there... I'm not sure that we need to protect calls in ->load_binary(), though - it's really unnecessary.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Look the swapout path. Without the big kernel lock you'll free vmas under swap_out().

	ftp://ftp.suse.com/pub/people/andrea/kernel-patches/pending-2.3.x/munmap-lock-1

Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

I checked to code in mm/*.c, and it seems that reading the vma-list is protected by either lock_kernel() [eg: swapper] or down(&mm->mmap_sem) [eg: do_mlock].

But this means that both locks are required if you modify the vma list. Single reader, multiple writer synchronization. Unusual, but interesting :-)

Unfortunately, it seems that this is often ignored, eg.

sys_mlock()->do_mlock()->merge_segments().
sys_brk()
sys_munmap() <<<<<< fixed by your patch.

It that correct?
Should I write a patch or is someone working on these problems? How should we fix it?

a) the swapper calls down(&mm->mmap_sem), but I guess that would lock-up.

b) everyone who changes the vma list calls lock_kernel().
I think it would be a bad thing to call lock_kernel() immediately in the sys_??() function, I think we should hide the lock_kernel() call somewhere
inside the vma-list code [add functions which modify the vma list, and they call lock_kernel()].

--
Manfred

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Hi,

The swapper relies on it being protected by the big lock. The mm semaphore is required when you need additional protection: specifically, if you need to sleep while manipulating the vma lists (eg. in page faults).

Correct, but you only need the one lock --- the big lock --- to read the vma list, which is what the swapper does. The swapper only needs write access to the page tables, not to the vma list.

Massive deadlock, indeed. We've looked at this but it is soooo painful.

... or an equivalent lock. The big lock itself isn't needed if we have a per-mm spinlock, but we do need something lighter weight than the mmap semaphore to let the swapper read-protect the vma lists.

--Stephen
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Hi,

Yes. The swapout code relies on the big lock to freeze the vma, and on the page_table_lock to protect the ptes, so that it can avoid worrying about the mm_sem at all.

If munmap ever drops vmas without the big lock, the swapper _will_ break. Making this into a per-mm lock would not be hard, btw.

--Stephen
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/