Skip to contentSkip to conversation

mlockall(MCL_FUTURE) unlocks currently locked mappings

PATCH In review Chris Wright 2 participants 7 messages Sep 29, 2004 – Oct 1, 2004

Subsystems (2)
Memory management Memory mapping
lore
Share & tools
lore

Chris Wright

author
#0 lore raw

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

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 <[email protected]>
mm/mlock.c renamed from mm/mlock.c~mcl_future +4 -3
@@ -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)
author
#1 lore raw

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

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

Signed-off-by Chris Wright <[email protected]>
mm/mlock.c renamed from mm/mlock.c~mcl_current_acct +2 -1
@@ -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);
author
#2 lore raw

PATCH 3/4 make can_do_mlock useful for mlock/mlockall

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 <[email protected]>
mm/mlock.c renamed from mm/mlock.c~can_do_mlock +8 -7
@@ -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;
author
#3 lore raw

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

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 <[email protected]>
mm/mlock.c renamed from mm/mlock.c~move_sem +4 -3
@@ -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;
 }
#4 lore raw
In reply to Chris Wright
Quoted 4 lines · 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.

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?

author
#5 lore raw
Quoted 6 lines · * Andrew Morton ([email protected]) wrote:
Quoted 4 lines · 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.

Quoted 8 lines

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

Signature · 1 line
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
author
#6 lore raw
Quoted 10 lines · * Chris Wright ([email protected]) wrote:
Quoted 6 lines · * Andrew Morton ([email protected]) 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

Signature · 1 line
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net