--- 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);
Hi Mark,
* Mark F. Haigh ([email protected]) 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
On Tue, 8 Feb 2005, Chris Wright wrote:
> * Mark F. Haigh ([email protected]) 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
* Hugh Dickins ([email protected]) 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
On Wed, 9 Feb 2005, Chris Wright wrote:
> * Hugh Dickins ([email protected]) 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
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
[email protected]