[PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Mark F. Haigh]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]


Unless I'm missing something, in kernel/fork.c, dup_mmap():

			if (security_vm_enough_memory(len))
				goto fail_nomem;
/* ... */
fail_nomem:
	retval = -ENOMEM;
	vm_unacct_memory(charge);
/* ... */

If security_vm_enough_memory() fails there, then we vm_unacct_memory() 
that we never accounted (if security_vm_enough_memory() fails, no memory 
is accounted).

If it is in fact a bug, a simple but largely untested patch (against 
2.6.11-rc3-bk5) is included.


Mark F. Haigh
Mark.Haigh@spirentcom.com


[-- Attachment #2: fork-patch --]
[-- Type: text/plain, Size: 526 bytes --]

--- linux-2.6.11-rc3-bk5/kernel/fork.c.orig	2005-02-08 19:12:26.254589504 -0800
+++ linux-2.6.11-rc3-bk5/kernel/fork.c	2005-02-08 19:16:30.756419576 -0800
@@ -193,8 +193,10 @@
 		charge = 0;
 		if (mpnt->vm_flags & VM_ACCOUNT) {
 			unsigned int len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
-			if (security_vm_enough_memory(len))
-				goto fail_nomem;
+			if (security_vm_enough_memory(len)) {
+				retval = -ENOMEM;
+				goto out;
+			}
 			charge = len;
 		}
 		tmp = kmem_cache_alloc(vm_area_cachep, SLAB_KERNEL);

[PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Mark F. Haigh]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

[Aargh!  Missing Signed-off-by.]

Unless I'm missing something, in kernel/fork.c, dup_mmap():

			if (security_vm_enough_memory(len))
				goto fail_nomem;
/* ... */
fail_nomem:
	retval = -ENOMEM;
	vm_unacct_memory(charge);
/* ... */

If security_vm_enough_memory() fails there, then we vm_unacct_memory()
that we never accounted (if security_vm_enough_memory() fails, no memory
is accounted).

If it is in fact a bug, a simple but largely untested patch (against
2.6.11-rc3-bk5) is included.


Mark F. Haigh
Mark.Haigh@spirentcom.com

Signed-off-by: Mark F. Haigh  <Mark.Haigh@spirentcom.com>


[-- Attachment #2: fork-patch --]
[-- Type: text/plain, Size: 527 bytes --]

--- linux-2.6.11-rc3-bk5/kernel/fork.c.orig	2005-02-08 19:12:26.254589504 -0800
+++ linux-2.6.11-rc3-bk5/kernel/fork.c	2005-02-08 19:16:30.756419576 -0800
@@ -193,8 +193,10 @@
 		charge = 0;
 		if (mpnt->vm_flags & VM_ACCOUNT) {
 			unsigned int len = (mpnt->vm_end - mpnt->vm_start) >> PAGE_SHIFT;
-			if (security_vm_enough_memory(len))
-				goto fail_nomem;
+			if (security_vm_enough_memory(len)) {
+				retval = -ENOMEM;
+				goto out;
+			}
 			charge = len;
 		}
 		tmp = kmem_cache_alloc(vm_area_cachep, SLAB_KERNEL);

Re: [PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Chris Wright]

Hi Mark,

* Mark F. Haigh (Mark.Haigh@SpirentCom.COM) wrote:
> [Aargh!  Missing Signed-off-by.]
> 
> Unless I'm missing something, in kernel/fork.c, dup_mmap():
> 
> 			if (security_vm_enough_memory(len))
> 				goto fail_nomem;
> /* ... */
> fail_nomem:
> 	retval = -ENOMEM;
> 	vm_unacct_memory(charge);
> /* ... */
> 
> If security_vm_enough_memory() fails there, then we vm_unacct_memory()
> that we never accounted (if security_vm_enough_memory() fails, no memory
> is accounted).

You missed one subtle point.  That failure case actually unaccts 0 pages
(note the use of charge).  Not the nicest, but I believe correct.

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

Re: [PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Hugh Dickins]

On Tue, 8 Feb 2005, Chris Wright wrote:
> * Mark F. Haigh (Mark.Haigh@SpirentCom.COM) wrote:
> > 
> > If security_vm_enough_memory() fails there, then we vm_unacct_memory()
> > that we never accounted (if security_vm_enough_memory() fails, no memory
> > is accounted).
> 
> You missed one subtle point.  That failure case actually unaccts 0 pages
> (note the use of charge).  Not the nicest, but I believe correct.

Not quite: Mark's patch is worse than unnecessary, it's wrong.

dup_mmap's charge starts out at 0 and gets added to each time around
the loop through vmas; if security_vm_enough_memory fails at any point
in that loop, we need to vm_unacct_memory the charge already accumulated.

Hugh

Re: [PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Chris Wright]

* Hugh Dickins (hugh@veritas.com) wrote:
> dup_mmap's charge starts out at 0 and gets added to each time around
> the loop through vmas; if security_vm_enough_memory fails at any point
> in that loop, we need to vm_unacct_memory the charge already accumulated.

If that's the requirement, then it's broken as is, because there is
nothing accumulating.  len is re-determined each pass, and charge is
reset each pass.  But I think that it's ok, as we only care about the
last pass.  If dup_mmap() fails part way through, the cleanup path should
call unaccount for the (potentially) accounted by not fully setup vma then
call exit_mmap() and clear all the vmas that got accounted for already.
Either way, Mark's patch is not needed, and I don't think anything needs
patching in this area.  Hugh, do you agree?

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

Re: [PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Hugh Dickins]

On Wed, 9 Feb 2005, Chris Wright wrote:

> * Hugh Dickins (hugh@veritas.com) wrote:
> > dup_mmap's charge starts out at 0 and gets added to each time around
> > the loop through vmas; if security_vm_enough_memory fails at any point
> > in that loop, we need to vm_unacct_memory the charge already accumulated.
> 
> If that's the requirement, then it's broken as is, because there is
> nothing accumulating.  len is re-determined each pass, and charge is
> reset each pass.

You're right, it's me who's wrong, sorry.  I was remembering how it
was before 2.6.7, when Oleg pointed out that it was doubly unaccounting
(and it's probably me who was guilty of that double unaccounting too).

> But I think that it's ok, as we only care about the
> last pass.  If dup_mmap() fails part way through, the cleanup path should
> call unaccount for the (potentially) accounted by not fully setup vma then
> call exit_mmap() and clear all the vmas that got accounted for already.

Yes, that was Oleg's point when he fixed it.

> Either way, Mark's patch is not needed, and I don't think anything needs
> patching in this area.  Hugh, do you agree?

Yes I agree - thanks for clearing that up, Chris.

Hugh

Re: [PATCH] kernel/fork.c: VM accounting bugfix (2.6.11-rc3-bk5)

[Mark F. Haigh]

Chris Wright wrote:
<snip>
> 
> You missed one subtle point.  That failure case actually unaccts 0 pages
> (note the use of charge).  Not the nicest, but I believe correct.
> 

Right.  I did miss that.  Thanks for the explanations, Chris and Hugh, I 
appreciate it.


Mark F. Haigh
Mark.Haigh@spirentcom.com