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

[Mark F. Haigh]

[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
***@spirentcom.com

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

[Chris Wright]

Hi Mark,

Post by Mark F. Haigh
[Aargh! Missing Signed-off-by.]
if (security_vm_enough_memory(len))
goto 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

[Hugh Dickins]

Post by Chris Wright

Post by Mark F. Haigh
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Chris Wright]

Post by Hugh Dickins
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

[Hugh Dickins]

Post by Chris Wright

Post by Hugh Dickins
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).

Post by Chris Wright
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.

Post by Chris Wright
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Mark F. Haigh]

Chris Wright wrote:
<snip>

Post by Chris Wright
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
***@spirentcom.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/