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]>
--- 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)
Only check memlock rlimit against mm->total_vm when mlockall() flags
include MCL_CURRENT.
Signed-off-by: Chris Wright <[email protected]>
--- 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(¤t->mm->mmap_sem);
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]>
--- 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(¤t->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;
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]>
--- 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(¤t->mm->mmap_sem);
return error;
@@ -167,7 +167,6 @@ asmlinkage long sys_mlockall(int flags)
unsigned long lock_limit;
int ret = -EINVAL;
- down_write(¤t->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(¤t->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(¤t->mm->mmap_sem);
+out:
return ret;
}
Chris Wright <[email protected]> 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?
* Andrew Morton ([email protected]) wrote:
> Chris Wright <[email protected]> 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
* Chris Wright ([email protected]) wrote:
> * 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net