Re: [PATCH] mm: overcommit updates

Re: [PATCH] mm: overcommit updates

[Nikita Danilov]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> writes:

> ChangeSet 1.2136.3.17, 2005/01/03 20:15:37-08:00, Andries.Brouwer@cwi.nl
>

[...]

>  
> +	/* Leave the last 3% for root */
> +	if (current->euid)
> +		allowed -= allowed / 32;

This results in

	/*
	 * Leave the last 3% for root
	 */
	if (!capable(CAP_SYS_ADMIN))
		allowed -= allowed / 32;
	allowed += total_swap_pages;

	/* Leave the last 3% for root */
	if (current->euid)
		allowed -= allowed / 32;

in security/commoncaps.c (and similarly in security/dummy.c). Why
"super-user" reservation is handled twice, and with that antiquated
current->euid check instead of capabilities? Broken merge?

On another account, shouldn't capable(CAP_SYS_ADMIN) checks in
cap_vm_enough_memory() be replaced with capable(CAP_SYS_RESOURCE):
(CAP_SYS_RESOURCE is used by file systems to control reserved disk
blocks)?

Nikita.

[PATCH] remove duplicated patch fragment

[Andries Brouwer]

On Tue, Jan 04, 2005 at 03:36:11PM +0300, Nikita Danilov wrote:

> 	/*
> 	 * Leave the last 3% for root
> 	 */
> 	if (!capable(CAP_SYS_ADMIN))
> 		allowed -= allowed / 32;
> 
> 	/* Leave the last 3% for root */
> 	if (current->euid)
> 		allowed -= allowed / 32;
> 
> in security/commoncaps.c (and similarly in security/dummy.c). Why
> "super-user" reservation is handled twice, and with that antiquated
> current->euid check instead of capabilities? Broken merge?

Yes - sorry. The first of these two semi-identical fragments
is from Alan and appeared in patch-2.6.9, two weeks after
the patch under discussion was made. So, the second half
can be dropped. Below a patch.

> On another account, shouldn't capable(CAP_SYS_ADMIN) checks in
> cap_vm_enough_memory() be replaced with capable(CAP_SYS_RESOURCE):
> (CAP_SYS_RESOURCE is used by file systems to control reserved disk
> blocks)?

The use of current->euid comes from the use of current->euid in dummy.c
a few lines higher up in the same routine.
The use of CAP_SYS_ADMIN comes from the use of CAP_SYS_ADMIN in
commoncap.c a few lines higher up in the same routine.

I have no strong opinion about what is best.

Andries

diff -uprN -X /linux/dontdiff a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c	2005-01-04 18:33:40.000000000 +0100
+++ b/security/commoncap.c	2005-01-04 18:35:49.000000000 +0100
@@ -386,10 +386,6 @@ int cap_vm_enough_memory(long pages)
 		allowed -= allowed / 32;
 	allowed += total_swap_pages;
 
-	/* Leave the last 3% for root */
-	if (current->euid)
-		allowed -= allowed / 32;
-
 	/* Don't let a single process grow too big:
 	   leave 3% of the size of this process for other processes */
 	allowed -= current->mm->total_vm / 32;

Re: [PATCH] mm: overcommit updates

[Alan Cox]

Looks like a broken merge to me. When the 3% trick was proposed I
rewrote it as capabilities and submitted it to Linus, now it looks like
some months later the original one has been regurgitated out of -mm

Re: [PATCH] remove duplicated patch fragment

[Chris Wright]

* Andries Brouwer (Andries.Brouwer@cwi.nl) wrote:
> Yes - sorry. The first of these two semi-identical fragments
> is from Alan and appeared in patch-2.6.9, two weeks after
> the patch under discussion was made. So, the second half
> can be dropped. Below a patch.

Yes, can this please be applied? ;-)

Acked-by: Chris Wright <chrisw@osdl.org>

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

Re: [PATCH] remove duplicated patch fragment

[Chris Wright]

* Andries Brouwer (Andries.Brouwer@cwi.nl) wrote:
> On Tue, Jan 04, 2005 at 03:36:11PM +0300, Nikita Danilov wrote:
> > On another account, shouldn't capable(CAP_SYS_ADMIN) checks in
> > cap_vm_enough_memory() be replaced with capable(CAP_SYS_RESOURCE):
> > (CAP_SYS_RESOURCE is used by file systems to control reserved disk
> > blocks)?
> 
> The use of current->euid comes from the use of current->euid in dummy.c
> a few lines higher up in the same routine.
> The use of CAP_SYS_ADMIN comes from the use of CAP_SYS_ADMIN in
> commoncap.c a few lines higher up in the same routine.
> 
> I have no strong opinion about what is best.

Unfortunately, this what committed on 2003/05/25 (IOW, it's been in
there since 2.5.70).  So, we can't really change that w/out possibly
breaking things.

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

Re: [PATCH] mm: overcommit updates

[Andries Brouwer]

On Tue, Jan 04, 2005 at 10:03:56PM +0000, Alan Cox wrote:

> Looks like a broken merge to me. When the 3% trick was proposed I
> rewrote it as capabilities and submitted it to Linus, now it looks like
> some months later the original one has been regurgitated out of -mm

No. The two semi-identical fragments have independent origins.

Andries