* RE: [PATCH] getrusage sucks
@ 2005-11-10 23:47 Hua Zhong (hzhong)
2005-11-11 0:23 ` Claudio Scordino
0 siblings, 1 reply; 24+ messages in thread
From: Hua Zhong (hzhong) @ 2005-11-10 23:47 UTC (permalink / raw)
To: Claudio Scordino, linux-kernel, kernelnewbies
The reason is what if tsk is no longer available when you call
getrusage?
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org
> [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
> Claudio Scordino
> Sent: Thursday, November 10, 2005 2:34 PM
> To: linux-kernel@vger.kernel.org; kernelnewbies@nl.linux.org
> Subject: [PATCH] getrusage sucks
>
> Does exist any _real_ reason why getrusage can't be invoked
> by a task to know
> statistics of another task ?
>
> The changes would be very trivial, as shown by the following patch.
>
> Claudio
>
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1746,9 +1746,13 @@ int getrusage(struct task_struct *p, int
>
> asmlinkage long sys_getrusage(int who, struct rusage __user *ru)
> {
> - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN)
> - return -EINVAL;
> - return getrusage(current, who, ru);
> + if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) {
> + struct task_struct* tsk = find_task_by_pid(who);
> + if (tsk == NULL)
> + return -EINVAL;
> + return getrusage(tsk, RUSAGE_SELF, ru);
> + } else
> + return getrusage(current, who, ru);
> }
>
> asmlinkage long sys_umask(int mask)
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] getrusage sucks 2005-11-10 23:47 [PATCH] getrusage sucks Hua Zhong (hzhong) @ 2005-11-11 0:23 ` Claudio Scordino 2005-11-11 0:32 ` Magnus Naeslund(f) 0 siblings, 1 reply; 24+ messages in thread From: Claudio Scordino @ 2005-11-11 0:23 UTC (permalink / raw) To: Hua Zhong (hzhong); +Cc: linux-kernel, kernelnewbies On Friday 11 November 2005 00:47, Hua Zhong (hzhong) wrote: > The reason is what if tsk is no longer available when you call > getrusage? Sorry, but honestly I don't see any problem: as you can see from my patch, if tsk is no longer available, getrusage returns -1 and sets errno appropriately (equal to EINVAL, which means that who is invalid). Claudio > > > -----Original Message----- > > > > Does exist any _real_ reason why getrusage can't be invoked > > by a task to know > > statistics of another task ? > > > > The changes would be very trivial, as shown by the following patch. > > > > Claudio > > > > > > diff --git a/kernel/sys.c b/kernel/sys.c > > --- a/kernel/sys.c > > +++ b/kernel/sys.c > > @@ -1746,9 +1746,13 @@ int getrusage(struct task_struct *p, int > > > > asmlinkage long sys_getrusage(int who, struct rusage __user *ru) > > { > > - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) > > - return -EINVAL; > > - return getrusage(current, who, ru); > > + if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) { > > + struct task_struct* tsk = find_task_by_pid(who); > > + if (tsk == NULL) > > + return -EINVAL; > > + return getrusage(tsk, RUSAGE_SELF, ru); > > + } else > > + return getrusage(current, who, ru); > > } > > > > asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 0:23 ` Claudio Scordino @ 2005-11-11 0:32 ` Magnus Naeslund(f) 2005-11-11 1:11 ` Claudio Scordino 0 siblings, 1 reply; 24+ messages in thread From: Magnus Naeslund(f) @ 2005-11-11 0:32 UTC (permalink / raw) To: Claudio Scordino; +Cc: Hua Zhong (hzhong), linux-kernel, kernelnewbies Claudio Scordino wrote: > On Friday 11 November 2005 00:47, Hua Zhong (hzhong) wrote: > >>The reason is what if tsk is no longer available when you call >>getrusage? > > > Sorry, but honestly I don't see any problem: as you can see from my patch, if > tsk is no longer available, getrusage returns -1 and sets errno appropriately > (equal to EINVAL, which means that who is invalid). > > Claudio > You need to wrap this with a read_lock(&tasklist_lock) to be safe, I think. Regards, Magnus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 0:32 ` Magnus Naeslund(f) @ 2005-11-11 1:11 ` Claudio Scordino 2005-11-11 13:30 ` Alan Cox 2005-11-15 1:08 ` Peter Chubb 0 siblings, 2 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-11 1:11 UTC (permalink / raw) To: Magnus Naeslund(f); +Cc: Hua Zhong (hzhong), linux-kernel, kernelnewbies > You need to wrap this with a read_lock(&tasklist_lock) to be safe, I think. Right. Probably this was the meaning also of Hua's mail. Sorry, but I didn't get it immediately. So, what if I do as follows ? Do you see any problem with this solution ? Many thanks, Claudio diff --git a/kernel/sys.c b/kernel/sys.c --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1746,9 +1746,22 @@ int getrusage(struct task_struct *p, int asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) - return -EINVAL; - return getrusage(current, who, ru); + int res; + if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) { + struct task_struct* tsk; + read_lock(&tasklist_lock); + tsk = find_task_by_pid(who); + if (tsk == NULL) { + read_unlock(&tasklist_lock); + return -EINVAL; + } + res = getrusage(tsk, RUSAGE_SELF, ru); + read_unlock(&tasklist_lock); + return res; + } else { + res = getrusage(current, who, ru); + return res; + } } asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 1:11 ` Claudio Scordino @ 2005-11-11 13:30 ` Alan Cox 2005-11-11 22:38 ` Claudio Scordino 2005-11-15 1:08 ` Peter Chubb 1 sibling, 1 reply; 24+ messages in thread From: Alan Cox @ 2005-11-11 13:30 UTC (permalink / raw) To: Claudio Scordino Cc: Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies On Gwe, 2005-11-11 at 02:11 +0100, Claudio Scordino wrote: > > You need to wrap this with a read_lock(&tasklist_lock) to be safe, I think. > > Right. Probably this was the meaning also of Hua's mail. Sorry, but I didn't > get it immediately. > > So, what if I do as follows ? Do you see any problem with this solution ? It will depend on the data accuracy. If the information on cpu usage is very accurate then it becomes a way to analyse what is happening to tasks you don't own - such as say cryptographic functions in the web server... Otherwise, or with an owner check I see no real problem with the concept ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 13:30 ` Alan Cox @ 2005-11-11 22:38 ` Claudio Scordino 2005-11-11 23:23 ` Alan Cox 2005-11-11 23:41 ` David Wagner 0 siblings, 2 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-11 22:38 UTC (permalink / raw) To: Alan Cox Cc: Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner > > > You need to wrap this with a read_lock(&tasklist_lock) to be safe, I > > > think. > > > > It will depend on the data accuracy. If the information on cpu usage is > very accurate then it becomes a way to analyse what is happening to > tasks you don't own - such as say cryptographic functions in the web > server... > > Otherwise, or with an owner check I see no real problem with the concept So, is the following patch right ? I've added both the lock and the owner check... Do you think it may be an interesting feature to be inserted in the kernel ? Many thanks, Claudio diff --git a/kernel/sys.c b/kernel/sys.c --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1746,9 +1746,25 @@ int getrusage(struct task_struct *p, int asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) - return -EINVAL; - return getrusage(current, who, ru); + if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) { + struct task_struct* tsk; + struct rusage r; + read_lock(&tasklist_lock); + tsk = find_task_by_pid(who); + if (tsk == NULL) { + read_unlock(&tasklist_lock); + return -EINVAL; + } + if ((current->euid != tsk->euid) && + (current->euid != tsk->uid)) { + read_unlock(&tasklist_lock); + return -EINVAL; + } + k_getrusage(tsk, RUSAGE_SELF, &r); + read_unlock(&tasklist_lock); + return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0; + } else + return getrusage(current, who, ru); } asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 22:38 ` Claudio Scordino @ 2005-11-11 23:23 ` Alan Cox 2005-11-11 23:02 ` Chris Wright 2005-11-11 23:08 ` [PATCH] getrusage sucks Claudio Scordino 2005-11-11 23:41 ` David Wagner 1 sibling, 2 replies; 24+ messages in thread From: Alan Cox @ 2005-11-11 23:23 UTC (permalink / raw) To: Claudio Scordino Cc: Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner On Gwe, 2005-11-11 at 23:38 +0100, Claudio Scordino wrote: > + if ((current->euid != tsk->euid) && > + (current->euid != tsk->uid)) { > + read_unlock(&tasklist_lock); > + return -EINVAL; Would be -EPERM also wants a 'privilege' check. Not sure which would be best here - CAP_SYS_ADMIN seems to be the 'default' used ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:23 ` Alan Cox @ 2005-11-11 23:02 ` Chris Wright 2005-11-11 23:44 ` David Wagner 2005-11-11 23:58 ` Alan Cox 2005-11-11 23:08 ` [PATCH] getrusage sucks Claudio Scordino 1 sibling, 2 replies; 24+ messages in thread From: Chris Wright @ 2005-11-11 23:02 UTC (permalink / raw) To: Alan Cox Cc: Claudio Scordino, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner * Alan Cox (alan@lxorguk.ukuu.org.uk) wrote: > On Gwe, 2005-11-11 at 23:38 +0100, Claudio Scordino wrote: > > + if ((current->euid != tsk->euid) && > > + (current->euid != tsk->uid)) { > > + read_unlock(&tasklist_lock); > > + return -EINVAL; > > > Would be -EPERM also wants a 'privilege' check. Not sure which would be > best here - CAP_SYS_ADMIN seems to be the 'default' used It's already available via /proc w/out protection. And ditto via posix cpu timers. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:02 ` Chris Wright @ 2005-11-11 23:44 ` David Wagner 2005-11-12 0:53 ` Chris Wright 2005-11-11 23:58 ` Alan Cox 1 sibling, 1 reply; 24+ messages in thread From: David Wagner @ 2005-11-11 23:44 UTC (permalink / raw) To: linux-kernel Chris Wright wrote: >It's already available via /proc w/out protection. And ditto via posix >cpu timers. If so, maybe that code should be fixed. Where exactly in /proc would I find the getrusage() info of another process? Is there any argument that disclosing it to everyone is safe? Or is it just that no one has ever given the security considerations much thought up till now? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:44 ` David Wagner @ 2005-11-12 0:53 ` Chris Wright 2005-11-12 6:37 ` David Wagner 0 siblings, 1 reply; 24+ messages in thread From: Chris Wright @ 2005-11-12 0:53 UTC (permalink / raw) To: David Wagner; +Cc: linux-kernel * David Wagner (daw@cs.berkeley.edu) wrote: > Chris Wright wrote: > >It's already available via /proc w/out protection. And ditto via posix > >cpu timers. > > If so, maybe that code should be fixed. Where exactly in /proc would > I find the getrusage() info of another process? Just from /proc/[pid]/stat. (fs/proc/array.c::do_task_stat). > Is there any argument > that disclosing it to everyone is safe? Or is it just that no one has > ever given the security considerations much thought up till now? I guess it keeps falling in the "too theoretical" category. It can be protected by policy, but default is open. thanks, -chris ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-12 0:53 ` Chris Wright @ 2005-11-12 6:37 ` David Wagner 0 siblings, 0 replies; 24+ messages in thread From: David Wagner @ 2005-11-12 6:37 UTC (permalink / raw) To: linux-kernel Chris Wright wrote: >/proc/[pid]/stat. (fs/proc/array.c::do_task_stat). > >> Is there any argument >> that disclosing it to everyone is safe? Or is it just that no one has >> ever given the security considerations much thought up till now? > >I guess it keeps falling in the "too theoretical" category. It can be >protected by policy, but default is open. Ahh, I see. I had never looked at /proc/[pid]/stat carefully before. Well, making /proc/[pid]/stat world-readable by default looks pretty dubious to me. There's all sorts of stuff there that I suspect should not be revealed: EIP, stack pointer, stats on paging and swapping, and so on. I suspect that this is not at all safe. Most crypto algorithms tend to fall apart when you have side channels like this. Maybe no one cares, because no one uses Linux in a multi-user setting where users are motivated to attack each other or attack the system. But baking this kind of "privilege escalation" vulnerability into the kernel by default doesn't seem like a good idea to me. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:02 ` Chris Wright 2005-11-11 23:44 ` David Wagner @ 2005-11-11 23:58 ` Alan Cox 2005-11-11 23:43 ` Claudio Scordino 1 sibling, 1 reply; 24+ messages in thread From: Alan Cox @ 2005-11-11 23:58 UTC (permalink / raw) To: Chris Wright Cc: Claudio Scordino, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner On Gwe, 2005-11-11 at 15:02 -0800, Chris Wright wrote: > * Alan Cox (alan@lxorguk.ukuu.org.uk) wrote: > > On Gwe, 2005-11-11 at 23:38 +0100, Claudio Scordino wrote: > > > + if ((current->euid != tsk->euid) && > > > + (current->euid != tsk->uid)) { > > > + read_unlock(&tasklist_lock); > > > + return -EINVAL; > > > > > > Would be -EPERM also wants a 'privilege' check. Not sure which would be > > best here - CAP_SYS_ADMIN seems to be the 'default' used > > It's already available via /proc w/out protection. And ditto via posix > cpu timers. In which case the only comment I have is the one about accuracy - and that is true for procfs too so will only come up if someone gets the urge to use perfctr timers for precision resource management ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:58 ` Alan Cox @ 2005-11-11 23:43 ` Claudio Scordino 2005-11-11 23:49 ` dean gaudet 0 siblings, 1 reply; 24+ messages in thread From: Claudio Scordino @ 2005-11-11 23:43 UTC (permalink / raw) To: Alan Cox Cc: Chris Wright, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner > > In which case the only comment I have is the one about accuracy - and > that is true for procfs too so will only come up if someone gets the > urge to use perfctr timers for precision resource management According to your comments, this the final patch. Should it be committed ? Claudio diff --git a/kernel/sys.c b/kernel/sys.c --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1746,9 +1746,26 @@ int getrusage(struct task_struct *p, int asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) - return -EINVAL; - return getrusage(current, who, ru); + if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) { + struct task_struct* tsk; + struct rusage r; + read_lock(&tasklist_lock); + tsk = find_task_by_pid(who); + if (tsk == NULL) { + read_unlock(&tasklist_lock); + return -EINVAL; + } + if ((current->euid != tsk->euid) && + (current->euid != tsk->uid) && + (!capable(CAP_SYS_ADMIN))){ + read_unlock(&tasklist_lock); + return -EPERM; + } + k_getrusage(tsk, RUSAGE_SELF, &r); + read_unlock(&tasklist_lock); + return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0; + } else + return getrusage(current, who, ru); } asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:43 ` Claudio Scordino @ 2005-11-11 23:49 ` dean gaudet 2005-11-12 1:10 ` Chris Wright 0 siblings, 1 reply; 24+ messages in thread From: dean gaudet @ 2005-11-11 23:49 UTC (permalink / raw) To: Claudio Scordino Cc: Alan Cox, Chris Wright, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner On Sat, 12 Nov 2005, Claudio Scordino wrote: > > > > In which case the only comment I have is the one about accuracy - and > > that is true for procfs too so will only come up if someone gets the > > urge to use perfctr timers for precision resource management > > According to your comments, this the final patch. this only lets you get RUSAGE_SELF data for the target... for many processes it's more important to get the RUSAGE_CHILDREN data... and really i'm having a hard time imagining a use for this code which on further inspection doesn't eventually blow up to the requirements of a proper accounting subsystem... (of which i understand there are two or three competining implementations in progress?) do you have a use case for this new code? -dean ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:49 ` dean gaudet @ 2005-11-12 1:10 ` Chris Wright 2005-11-12 15:10 ` making makefile for 2.6 kernel anil dahiya ` (4 more replies) 0 siblings, 5 replies; 24+ messages in thread From: Chris Wright @ 2005-11-12 1:10 UTC (permalink / raw) To: dean gaudet Cc: Claudio Scordino, Alan Cox, Chris Wright, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner * dean gaudet (dean-list-linux-kernel@arctic.org) wrote: > do you have a use case for this new code? I'm with Dean. What problem are you trying to solve? thanks, -chris ^ permalink raw reply [flat|nested] 24+ messages in thread
* making makefile for 2.6 kernel 2005-11-12 1:10 ` Chris Wright @ 2005-11-12 15:10 ` anil dahiya 2005-11-12 15:16 ` anil dahiya ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: anil dahiya @ 2005-11-12 15:10 UTC (permalink / raw) To: linux-kernel, kernelnewbies --- Chris Wright <chrisw@osdl.org> wrote: > * dean gaudet (dean-list-linux-kernel@arctic.org) > wrote: > > do you have a use case for this new code? > > I'm with Dean. What problem are you trying to > solve? > > thanks, > -chris > > -- > Kernelnewbies: Help each other learn about the Linux > kernel. > Archive: > http://mail.nl.linux.org/kernelnewbies/ > FAQ: http://kernelnewbies.org/faq/ > > __________________________________ Yahoo! Mail - PC Magazine Editors' Choice 2005 http://mail.yahoo.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* making makefile for 2.6 kernel 2005-11-12 1:10 ` Chris Wright 2005-11-12 15:10 ` making makefile for 2.6 kernel anil dahiya @ 2005-11-12 15:16 ` anil dahiya 2005-11-12 22:19 ` Sam Ravnborg 2005-11-13 1:34 ` [PATCH] getrusage sucks Claudio Scordino ` (2 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: anil dahiya @ 2005-11-12 15:16 UTC (permalink / raw) To: linux-kernel, kernelnewbies Hi I want to makefile for my kernel module.. 1.c 2.c 3.c files are places in /home/anil folder but these files contain .h (hearder)files from 3 different directory 1) /home/include 2) /root/incluent 3) /opt/include can u suggest me a makefile to generate a common module target.ko using these .C and .h files. Thanks in advance Regards, anil __________________________________ Yahoo! FareChase: Search multiple travel sites in one click. http://farechase.yahoo.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: making makefile for 2.6 kernel 2005-11-12 15:16 ` anil dahiya @ 2005-11-12 22:19 ` Sam Ravnborg 0 siblings, 0 replies; 24+ messages in thread From: Sam Ravnborg @ 2005-11-12 22:19 UTC (permalink / raw) To: anil dahiya; +Cc: linux-kernel, kernelnewbies On Sat, Nov 12, 2005 at 07:16:52AM -0800, anil dahiya wrote: > Hi > I want to makefile for my kernel module.. > 1.c 2.c 3.c files are places in /home/anil folder but > these files contain .h (hearder)files from 3 different > directory 1) /home/include 2) /root/incluent 3) > /opt/include > > can u suggest me a makefile to generate a common > module target.ko using these .C and .h files. Makefile obj-m := foo.o foo-y := 1.o 2.o 3.o EXTRA_CFLGAS := -I/home/include -I/root/incluent -I/opt/include And then build the module with: make -C $KERNEL_SRC M=`pwd` See Documentation/kbuild/makefiles.txt for reference. Note: Kernel being pointed out must be a fully build kernel Sam ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-12 1:10 ` Chris Wright 2005-11-12 15:10 ` making makefile for 2.6 kernel anil dahiya 2005-11-12 15:16 ` anil dahiya @ 2005-11-13 1:34 ` Claudio Scordino 2005-11-15 16:56 ` Claudio Scordino 2005-11-15 17:00 ` New getrusage Claudio Scordino 4 siblings, 0 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-13 1:34 UTC (permalink / raw) To: Chris Wright Cc: dean gaudet, Alan Cox, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner, Lee Revell On Saturday 12 November 2005 02:10, Chris Wright wrote: > * dean gaudet (dean-list-linux-kernel@arctic.org) wrote: > > do you have a use case for this new code? > > I'm with Dean. What problem are you trying to solve? I just want to improve getrusage to account for the case in which a server process needs to have usage information about client processes at run-time. In this case RUSAGE_SELF is more important than RUSAGE_CHILDREN. I think it would be an useful feature for some user-level applications. At the beginning, I didn't want to propose any different prototype for the function, even if I think that a more general (and correct) one would be int getrusage(int who, struct rusage *usage, pid_t pid); which accounts for all the situations we discussed so far. However, I've added a more restrictive check (as asked by David) and the goto proposed by Hua. Is now the patch right ? Do you think that it's useful ? Many thanks, Claudio diff --git a/kernel/sys.c b/kernel/sys.c --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1746,9 +1746,29 @@ int getrusage(struct task_struct *p, int asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) - return -EINVAL; - return getrusage(current, who, ru); + struct rusage r; + struct task_struct* tsk = current; + read_lock(&tasklist_lock); + if ((who != RUSAGE_SELF) && (who != RUSAGE_CHILDREN)) { + tsk = find_task_by_pid(who); + if ((tsk == NULL) || (who <=0)) + goto bad; + if (((current->uid != tsk->euid) || + (current->uid != tsk->suid) || + (current->uid != tsk->uid) || + (current->gid != tsk->egid) || + (current->gid != tsk->sgid) || + (current->gid != tsk->gid)) && !capable(CAP_SYS_PTRACE)) + goto bad; + who = RUSAGE_SELF; + } + k_getrusage(tsk, who, &r); + read_unlock(&tasklist_lock); + return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0; + + bad: + read_unlock(&tasklist_lock); + return tsk ? -EPERM : -EINVAL; } asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-12 1:10 ` Chris Wright ` (2 preceding siblings ...) 2005-11-13 1:34 ` [PATCH] getrusage sucks Claudio Scordino @ 2005-11-15 16:56 ` Claudio Scordino 2005-11-15 17:00 ` New getrusage Claudio Scordino 4 siblings, 0 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-15 16:56 UTC (permalink / raw) To: Peter Chubb Cc: Chris Wright, dean gaudet, Alan Cox, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner, Lee Revell On Tuesday 15 November 2005 02:08, Peter Chubb wrote: > >> You need to wrap this with a read_lock(&tasklist_lock) to be safe, > >> I think. > > Claudio> Right. Probably this was the meaning also of Hua's > Claudio> mail. Sorry, but I didn't get it immediately. > > Claudio> So, what if I do as follows ? Do you see any problem with > Claudio> this solution ? > > You should probably restrict the ability to read a process's usage to > a suitably privileged user -- i.e., effective uid same as the task's, > or capable(CAP_SYS_RESOURCE) or maybe capable(CAP_SYS_ADMIN) So, is CAP_SYS_PTRACE (as done in the patch below) not enough ? Honestly, I don't see any problem in allowing any user to know usage information about _his_ processes... Many thanks, Claudio Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com> diff --git a/kernel/sys.c b/kernel/sys.c --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1746,9 +1746,29 @@ int getrusage(struct task_struct *p, int asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { - if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) - return -EINVAL; - return getrusage(current, who, ru); + struct rusage r; + struct task_struct* tsk = current; + read_lock(&tasklist_lock); + if ((who != RUSAGE_SELF) && (who != RUSAGE_CHILDREN)) { + tsk = find_task_by_pid(who); + if ((tsk == NULL) || (who <=0)) + goto bad; + if (((current->uid != tsk->euid) || + (current->uid != tsk->suid) || + (current->uid != tsk->uid) || + (current->gid != tsk->egid) || + (current->gid != tsk->sgid) || + (current->gid != tsk->gid)) && !capable(CAP_SYS_PTRACE)) + goto bad; + who = RUSAGE_SELF; + } + k_getrusage(tsk, who, &r); + read_unlock(&tasklist_lock); + return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0; + + bad: + read_unlock(&tasklist_lock); + return tsk ? -EPERM : -EINVAL; } asmlinkage long sys_umask(int mask) ^ permalink raw reply [flat|nested] 24+ messages in thread
* New getrusage 2005-11-12 1:10 ` Chris Wright ` (3 preceding siblings ...) 2005-11-15 16:56 ` Claudio Scordino @ 2005-11-15 17:00 ` Claudio Scordino 4 siblings, 0 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-15 17:00 UTC (permalink / raw) To: Peter Chubb Cc: Chris Wright, dean gaudet, Alan Cox, Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner, Lee Revell Actually, I think that a better implementation of getrusage would be the following one, but it requires a (new) third parameter. That's why I didn't suggest it... Many thanks, Claudio Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com> asmlinkage long sys_getrusage(int who, struct rusage __user *ru, pid_t pid) { struct rusage r; struct task_struct* tsk = current; if ((pid < 0) || ((who != RUSAGE_SELF) && (who != RUSAGE_CHILDREN))) return -EINVAL; read_lock(&tasklist_lock); if (pid > 0) { tsk = find_task_by_pid(pid); if (tsk == NULL) goto bad; if (((current->uid != tsk->euid) || (current->uid != tsk->suid) || (current->uid != tsk->uid) || (current->gid != tsk->egid) || (current->gid != tsk->sgid) || (current->gid != tsk->gid)) && !capable(CAP_SYS_PTRACE)) goto bad; } k_getrusage(tsk, who, &r); read_unlock(&tasklist_lock); return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0; bad: read_unlock(&tasklist_lock); return tsk ? -EPERM : -EINVAL; } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 23:23 ` Alan Cox 2005-11-11 23:02 ` Chris Wright @ 2005-11-11 23:08 ` Claudio Scordino 1 sibling, 0 replies; 24+ messages in thread From: Claudio Scordino @ 2005-11-11 23:08 UTC (permalink / raw) To: Alan Cox Cc: Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies, David Wagner On Saturday 12 November 2005 00:23, Alan Cox wrote: > On Gwe, 2005-11-11 at 23:38 +0100, Claudio Scordino wrote: > > + if ((current->euid != tsk->euid) && > > + (current->euid != tsk->uid)) { > > + read_unlock(&tasklist_lock); > > + return -EINVAL; > > Would be -EPERM also wants a 'privilege' check. Not sure which would be > best here - CAP_SYS_ADMIN seems to be the 'default' used I would say -EPERM. Any other comment about the patch ? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 22:38 ` Claudio Scordino 2005-11-11 23:23 ` Alan Cox @ 2005-11-11 23:41 ` David Wagner 1 sibling, 0 replies; 24+ messages in thread From: David Wagner @ 2005-11-11 23:41 UTC (permalink / raw) To: linux-kernel Claudio Scordino wrote: >So, is the following patch right ? I've added both the lock and the owner >check... I think this patch is too permissive. It lets me run a setuid-root program and then call getrusage() on it. That's not a good idea. (I might, say, run /bin/su and then mount a timing or cache side-channel attacks on its password hashing code. That particular example might or might not work, but I hope it illustrates my concern.) Should you be using the same permission check that ptrace() does? ptrace() is more restrictive than what you seemed to have in mind. However, if ptrace() lets you attach to a process, then it's probably pretty safe to let you call getrusage(), as you could have just used ptrace() to read the process's entire memory image. kernel/ptrace.c:ptrace_may_attach() might be relevant. TOCTTOU vulnerabilities could be a problem. If I understand correctly, your locking code should take care of this, but you might want to double-check, as I'm not very familiar with the kernel's locking scheme. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] getrusage sucks 2005-11-11 1:11 ` Claudio Scordino 2005-11-11 13:30 ` Alan Cox @ 2005-11-15 1:08 ` Peter Chubb 1 sibling, 0 replies; 24+ messages in thread From: Peter Chubb @ 2005-11-15 1:08 UTC (permalink / raw) To: Claudio Scordino Cc: Magnus Naeslund(f), Hua Zhong (hzhong), linux-kernel, kernelnewbies >>>>> "Claudio" == Claudio Scordino <cloud.of.andor@gmail.com> writes: >> You need to wrap this with a read_lock(&tasklist_lock) to be safe, >> I think. Claudio> Right. Probably this was the meaning also of Hua's Claudio> mail. Sorry, but I didn't get it immediately. Claudio> So, what if I do as follows ? Do you see any problem with Claudio> this solution ? You should probably restrict the ability to read a process's usage to a suitably privileged user -- i.e., effective uid same as the task's, or capable(CAP_SYS_RESOURCE) or maybe capable(CAP_SYS_ADMIN) -- Dr Peter Chubb http://www.gelato.unsw.edu.au peterc AT gelato.unsw.edu.au The technical we do immediately, the political takes *forever* ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2005-11-15 17:00 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-11-10 23:47 [PATCH] getrusage sucks Hua Zhong (hzhong) 2005-11-11 0:23 ` Claudio Scordino 2005-11-11 0:32 ` Magnus Naeslund(f) 2005-11-11 1:11 ` Claudio Scordino 2005-11-11 13:30 ` Alan Cox 2005-11-11 22:38 ` Claudio Scordino 2005-11-11 23:23 ` Alan Cox 2005-11-11 23:02 ` Chris Wright 2005-11-11 23:44 ` David Wagner 2005-11-12 0:53 ` Chris Wright 2005-11-12 6:37 ` David Wagner 2005-11-11 23:58 ` Alan Cox 2005-11-11 23:43 ` Claudio Scordino 2005-11-11 23:49 ` dean gaudet 2005-11-12 1:10 ` Chris Wright 2005-11-12 15:10 ` making makefile for 2.6 kernel anil dahiya 2005-11-12 15:16 ` anil dahiya 2005-11-12 22:19 ` Sam Ravnborg 2005-11-13 1:34 ` [PATCH] getrusage sucks Claudio Scordino 2005-11-15 16:56 ` Claudio Scordino 2005-11-15 17:00 ` New getrusage Claudio Scordino 2005-11-11 23:08 ` [PATCH] getrusage sucks Claudio Scordino 2005-11-11 23:41 ` David Wagner 2005-11-15 1:08 ` Peter Chubb
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome