[PATCH 1/4] mlockall(MCL_FUTURE) unlocks currently locked mappings

[PATCH 1/4] mlockall(MCL_FUTURE) unlocks currently locked mappings

[Chris Wright]

Calling mlockall(MCL_FUTURE) will erroneously unlock any currently locked
mappings.  Fix this up, and while we're at it, remove the essentially
unused error variable.

Signed-off-by: Chris Wright <chrisw@osdl.org>

--- 2.6.9-rc2/mm/mlock.c~mcl_future	2004-09-28 15:00:11.781887352 -0700
+++ 2.6.9-rc2/mm/mlock.c	2004-09-28 15:27:01.784129808 -0700
@@ -138,7 +138,6 @@ asmlinkage long sys_munlock(unsigned lon
 
 static int do_mlockall(int flags)
 {
-	int error;
 	unsigned int def_flags;
 	struct vm_area_struct * vma;
 
@@ -149,8 +148,9 @@ static int do_mlockall(int flags)
 	if (flags & MCL_FUTURE)
 		def_flags = VM_LOCKED;
 	current->mm->def_flags = def_flags;
+	if (flags == MCL_FUTURE)
+		goto out;
 
-	error = 0;
 	for (vma = current->mm->mmap; vma ; vma = vma->vm_next) {
 		unsigned int newflags;
 
@@ -161,7 +161,8 @@ static int do_mlockall(int flags)
 		/* Ignore errors */
 		mlock_fixup(vma, vma->vm_start, vma->vm_end, newflags);
 	}
-	return error;
+out:
+	return 0;
 }
 
 asmlinkage long sys_mlockall(int flags)

[PATCH 2/4] mlockall() check rlimit only when MCL_CURRENT is set

[Chris Wright]

Only check memlock rlimit against mm->total_vm when mlockall() flags
include MCL_CURRENT.

Signed-off-by: Chris Wright <chrisw@osdl.org>

--- 2.6.9-rc2/mm/mlock.c~mcl_current_acct	2004-09-28 15:00:48.962235080 -0700
+++ 2.6.9-rc2/mm/mlock.c	2004-09-28 15:03:35.192964168 -0700
@@ -178,7 +178,8 @@ asmlinkage long sys_mlockall(int flags)
 	lock_limit >>= PAGE_SHIFT;
 
 	ret = -ENOMEM;
-	if ((current->mm->total_vm <= lock_limit) || capable(CAP_IPC_LOCK))
+	if (!(flags & MCL_CURRENT) || (current->mm->total_vm <= lock_limit) ||
+	    capable(CAP_IPC_LOCK))
 		ret = do_mlockall(flags);
 out:
 	up_write(&current->mm->mmap_sem);

[PATCH 3/4] make can_do_mlock useful for mlock/mlockall

[Chris Wright]

Move the simple can_do_mlock() check before the full rlimits based
restriction checks for mlock() and mlockall().  As it is, the check
adds nothing.  This has a side-effect of eliminating an unnecessary call
to can_do_mlock() on the munlockall() path.

Signed-off-by: Chris Wright <chrisw@osdl.org>

--- 2.6.9-rc2/mm/mlock.c~can_do_mlock	2004-09-28 15:06:54.668639256 -0700
+++ 2.6.9-rc2/mm/mlock.c	2004-09-28 15:08:56.175167456 -0700
@@ -60,8 +60,6 @@ static int do_mlock(unsigned long start,
 	struct vm_area_struct * vma, * next;
 	int error;
 
-	if (on && !can_do_mlock())
-		return -EPERM;
 	len = PAGE_ALIGN(len);
 	end = start + len;
 	if (end < start)
@@ -107,6 +105,9 @@ asmlinkage long sys_mlock(unsigned long 
 	unsigned long lock_limit;
 	int error = -ENOMEM;
 
+	if (!can_do_mlock())
+		return -EPERM;
+
 	down_write(&current->mm->mmap_sem);
 	len = PAGE_ALIGN(len + (start & ~PAGE_MASK));
 	start &= PAGE_MASK;
@@ -138,13 +139,9 @@ asmlinkage long sys_munlock(unsigned lon
 
 static int do_mlockall(int flags)
 {
-	unsigned int def_flags;
 	struct vm_area_struct * vma;
+	unsigned int def_flags = 0;
 
-	if (!can_do_mlock())
-		return -EPERM;
-
-	def_flags = 0;
 	if (flags & MCL_FUTURE)
 		def_flags = VM_LOCKED;
 	current->mm->def_flags = def_flags;
@@ -174,6 +171,10 @@ asmlinkage long sys_mlockall(int flags)
 	if (!flags || (flags & ~(MCL_CURRENT | MCL_FUTURE)))
 		goto out;
 
+	ret = -EPERM;
+	if (!can_do_mlock())
+		goto out;
+
 	lock_limit = current->rlim[RLIMIT_MEMLOCK].rlim_cur;
 	lock_limit >>= PAGE_SHIFT;
 

[PATCH 4/4] mlockall() take mmap_sem a bit later

[Chris Wright]

In sys_mlockall(), flags validation and can_do_mlock() check don't
require holding mmap_sem.  Move down_write() down a bit, and adjust
appropriately.

Signed-off-by: Chris Wright <chrisw@osdl.org>

--- 2.6.9-rc2/mm/mlock.c~move_sem	2004-09-28 23:46:01.000000000 -0700
+++ 2.6.9-rc2/mm/mlock.c	2004-09-29 00:22:25.625969360 -0700
@@ -119,7 +119,7 @@ asmlinkage long sys_mlock(unsigned long 
 	lock_limit >>= PAGE_SHIFT;
 
 	/* check against resource limits */
-	if ( (locked <= lock_limit) || capable(CAP_IPC_LOCK))
+	if ((locked <= lock_limit) || capable(CAP_IPC_LOCK))
 		error = do_mlock(start, len, 1);
 	up_write(&current->mm->mmap_sem);
 	return error;
@@ -167,7 +167,6 @@ asmlinkage long sys_mlockall(int flags)
 	unsigned long lock_limit;
 	int ret = -EINVAL;
 
-	down_write(&current->mm->mmap_sem);
 	if (!flags || (flags & ~(MCL_CURRENT | MCL_FUTURE)))
 		goto out;
 
@@ -175,6 +174,8 @@ asmlinkage long sys_mlockall(int flags)
 	if (!can_do_mlock())
 		goto out;
 
+	down_write(&current->mm->mmap_sem);
+
 	lock_limit = current->rlim[RLIMIT_MEMLOCK].rlim_cur;
 	lock_limit >>= PAGE_SHIFT;
 
@@ -182,8 +183,8 @@ asmlinkage long sys_mlockall(int flags)
 	if (!(flags & MCL_CURRENT) || (current->mm->total_vm <= lock_limit) ||
 	    capable(CAP_IPC_LOCK))
 		ret = do_mlockall(flags);
-out:
 	up_write(&current->mm->mmap_sem);
+out:
 	return ret;
 }
 

Re: [PATCH 1/4] mlockall(MCL_FUTURE) unlocks currently locked mappings

[Andrew Morton]

Chris Wright <chrisw@osdl.org> wrote:
>
> Calling mlockall(MCL_FUTURE) will erroneously unlock any currently locked
> mappings.  Fix this up, and while we're at it, remove the essentially
> unused error variable.

eek.

I've always assumed that mlockall(MCL_FUTURE) pins all your current pages
as well as future ones.  But no, that's what MCL_CURRENT|MCL_FUTURE does.

So when we fix this bug, we'll break my buggy test apps.

I wonder what other apps we'll break?

Re: [PATCH 1/4] mlockall(MCL_FUTURE) unlocks currently locked mappings

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Chris Wright <chrisw@osdl.org> wrote:
> >
> > Calling mlockall(MCL_FUTURE) will erroneously unlock any currently locked
> > mappings.  Fix this up, and while we're at it, remove the essentially
> > unused error variable.
> 
> eek.
> 
> I've always assumed that mlockall(MCL_FUTURE) pins all your current pages
> as well as future ones.  But no, that's what MCL_CURRENT|MCL_FUTURE does.
> 
> So when we fix this bug, we'll break my buggy test apps.
> 
> I wonder what other apps we'll break?

I don't think it will break apps.  The only difference is that it won't
unlock already locked mappings.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH 1/4] mlockall(MCL_FUTURE) unlocks currently locked mappings

[Chris Wright]

* Chris Wright (chrisw@osdl.org) wrote:
> * Andrew Morton (akpm@osdl.org) wrote:
> > I've always assumed that mlockall(MCL_FUTURE) pins all your current pages
> > as well as future ones.  But no, that's what MCL_CURRENT|MCL_FUTURE does.
> > 
> > So when we fix this bug, we'll break my buggy test apps.
> > 
> > I wonder what other apps we'll break?
> 
> I don't think it will break apps.  The only difference is that it won't
> unlock already locked mappings.

For example, I have a test that locks some pages, then calls
mlockall(MCL_FUTURE).  Resutls in unlocking all locked pages.

# pages locked, before MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:      1244 kB
# after MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:         0 kB

And, with just a simple call to MCL_FUTURE:
# before MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:         0 kB
# after MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:         0 kB

Now with the patch:

# pages locked, before MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:      1244 kB
# after MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:      1244 kB
	    ^^^^^^^
That's the only differenece, and I believe fixes a real bug whose
existance is more likely to surprise apps than it's squashing ;-)

And, again (unchanged) with just a simple call to MCL_FUTURE:
# before MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:         0 kB
# after MCL_FUTURE
$ cat /proc/`pidof mlockall`/status | grep VmLck
VmLck:         0 kB

The crux of the problem is in do_mlockall() in the for loop:

                newflags = vma->vm_flags | VM_LOCKED;
                if (!(flags & MCL_CURRENT))
                        newflags &= ~VM_LOCKED;
                mlock_fixup(vma, vma->vm_start, vma->vm_end, newflags);

So, for flags == MCL_FUTURE only, might as well bail out before the loop,
since MCL_FUTURE does nothing w.r.t. current mappings.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net