2005-01-04 12:36:34

by Nikita Danilov

[permalink] [raw]
Subject: Re: [PATCH] mm: overcommit updates

Linux Kernel Mailing List <[email protected]> writes:

> ChangeSet 1.2136.3.17, 2005/01/03 20:15:37-08:00, [email protected]
>

[...]

>
> + /* 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.


2005-01-04 17:27:58

by Andries E. Brouwer

[permalink] [raw]
Subject: [PATCH] remove duplicated patch fragment

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;


2005-01-04 22:38:50

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH] remove duplicated patch fragment

* Andries Brouwer ([email protected]) 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

2005-01-04 22:23:03

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH] remove duplicated patch fragment

* Andries Brouwer ([email protected]) 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 <[email protected]>

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

2005-01-04 23:21:50

by Alan

[permalink] [raw]
Subject: Re: [PATCH] mm: overcommit updates

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

2005-01-04 23:42:06

by Andries E. Brouwer

[permalink] [raw]
Subject: Re: [PATCH] mm: overcommit updates

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