[KJ] [PATCH] Extending getrusage

[KJ] [PATCH] Extending getrusage

[Claudio Scordino]

For the people who missed the beginning of the discussion, the
following patch is an extension of the existing getrusage syscall()
and it applies to the 2.6.16.9 kernel.

It allows a task to read usage information about another task. The argument
who can be equal to RUSAGE_SELF, to RUSAGE_CHILDREN or to a valid pid.

The permissions are checked through security_ptrace() as suggested by Andy.

Any other comment ?

Thanks,

           Claudio


Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com>
--- sys.old.c	2006-04-19 02:10:14.000000000 -0400
+++ sys.c	2006-04-20 10:53:16.000000000 -0400
@@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
 	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
 }

+/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
 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)) {
+		if (who <= 0)
+			goto bad;
+		tsk = find_task_by_pid(who);
+		if (tsk = NULL)
+			goto bad;
+		if ((tsk != current) && security_ptrace(current, tsk))
+			goto bad;
+		/* current can get info about tsk */
+		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)

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[PATCH] Extending getrusage

[Claudio Scordino]

For the people who missed the beginning of the discussion, the
following patch is an extension of the existing getrusage syscall()
and it applies to the 2.6.16.9 kernel.

It allows a task to read usage information about another task. The argument
who can be equal to RUSAGE_SELF, to RUSAGE_CHILDREN or to a valid pid.

The permissions are checked through security_ptrace() as suggested by Andy.

Any other comment ?

Thanks,

           Claudio


Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com>
--- sys.old.c	2006-04-19 02:10:14.000000000 -0400
+++ sys.c	2006-04-20 10:53:16.000000000 -0400
@@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
 	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
 }

+/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
 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)) {
+		if (who <= 0)
+			goto bad;
+		tsk = find_task_by_pid(who);
+		if (tsk == NULL)
+			goto bad;
+		if ((tsk != current) && security_ptrace(current, tsk))
+			goto bad;
+		/* current can get info about tsk */
+		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)

[KJ] Re: [PATCH] Extending getrusage

[Andrew Morton]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

"Claudio Scordino" <cloud.of.andor@gmail.com> wrote:
>
> For the people who missed the beginning of the discussion, the
> following patch is an extension of the existing getrusage syscall()
> and it applies to the 2.6.16.9 kernel.
> 
> It allows a task to read usage information about another task. The argument
> who can be equal to RUSAGE_SELF, to RUSAGE_CHILDREN or to a valid pid.
> 
> The permissions are checked through security_ptrace() as suggested by Andy.
> 

Bit hacky, but given the chosen values of RUSAGE_*, it seems solid enough.

There is no way of doing getrusage of another process and its children.

> --- sys.old.c	2006-04-19 02:10:14.000000000 -0400
> +++ sys.c	2006-04-20 10:53:16.000000000 -0400

Please prepare patches in `patch -p1' form, as per
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt.

> @@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
>  	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
>  }
> 
> +/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
>  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;

should be

	struct task_struct *tsk = current;

> +	read_lock(&tasklist_lock);
> +	if ((who != RUSAGE_SELF) && (who != RUSAGE_CHILDREN)) {

The parenthesisation is perhaps a little excessive.

> +		if (who <= 0)
> +			goto bad;
> +		tsk = find_task_by_pid(who);
> +		if (tsk == NULL)
> +			goto bad;
> +		if ((tsk != current) && security_ptrace(current, tsk))
> +			goto bad;
> +		/* current can get info about tsk */
> +		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;
>  }
> 

This patch changes sys_getrusage() but not getrusage().  But there are
several callers of getrusage() in various dark corners of the kernel.  Why
do they not also want the extended functionality?

I'd be reluctant to support this change without a compelling description of
why we actually want it.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] Extending getrusage

[Andrew Morton]

"Claudio Scordino" <cloud.of.andor@gmail.com> wrote:
>
> For the people who missed the beginning of the discussion, the
> following patch is an extension of the existing getrusage syscall()
> and it applies to the 2.6.16.9 kernel.
> 
> It allows a task to read usage information about another task. The argument
> who can be equal to RUSAGE_SELF, to RUSAGE_CHILDREN or to a valid pid.
> 
> The permissions are checked through security_ptrace() as suggested by Andy.
> 

Bit hacky, but given the chosen values of RUSAGE_*, it seems solid enough.

There is no way of doing getrusage of another process and its children.

> --- sys.old.c	2006-04-19 02:10:14.000000000 -0400
> +++ sys.c	2006-04-20 10:53:16.000000000 -0400

Please prepare patches in `patch -p1' form, as per
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt.

> @@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
>  	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
>  }
> 
> +/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
>  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;

should be

	struct task_struct *tsk = current;

> +	read_lock(&tasklist_lock);
> +	if ((who != RUSAGE_SELF) && (who != RUSAGE_CHILDREN)) {

The parenthesisation is perhaps a little excessive.

> +		if (who <= 0)
> +			goto bad;
> +		tsk = find_task_by_pid(who);
> +		if (tsk == NULL)
> +			goto bad;
> +		if ((tsk != current) && security_ptrace(current, tsk))
> +			goto bad;
> +		/* current can get info about tsk */
> +		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;
>  }
> 

This patch changes sys_getrusage() but not getrusage().  But there are
several callers of getrusage() in various dark corners of the kernel.  Why
do they not also want the extended functionality?

I'd be reluctant to support this change without a compelling description of
why we actually want it.

[KJ] Re: [PATCH] Extending getrusage

[bert hubert]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Apr 20, 2006 at 04:21:40PM -0700, Andrew Morton wrote:
> I'd be reluctant to support this change without a compelling description of
> why we actually want it.

It offers nothing that isn't available elsewhere (I think), except more
cheaply. It also has the potential to be multiplatform one day. Right now to
get the CPU usage of a random pid, one has to implement the equivalent of
/proc/ parsing for each platform.

If at least Linux did 'getrusage for foreign pid', that would clean up
things up somewhat already.

It might even mean a 'portable top', and who knows, more unixes might
follow.

I for one would save work having a getrusage that does this.

	Bert

-- 
http://www.PowerDNS.com      Open source, database driven DNS Software 
http://netherlabs.nl              Open and Closed source services

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] Extending getrusage

[bert hubert]

On Thu, Apr 20, 2006 at 04:21:40PM -0700, Andrew Morton wrote:
> I'd be reluctant to support this change without a compelling description of
> why we actually want it.

It offers nothing that isn't available elsewhere (I think), except more
cheaply. It also has the potential to be multiplatform one day. Right now to
get the CPU usage of a random pid, one has to implement the equivalent of
/proc/ parsing for each platform.

If at least Linux did 'getrusage for foreign pid', that would clean up
things up somewhat already.

It might even mean a 'portable top', and who knows, more unixes might
follow.

I for one would save work having a getrusage that does this.

	Bert

-- 
http://www.PowerDNS.com      Open source, database driven DNS Software 
http://netherlabs.nl              Open and Closed source services

[KJ] Re: [PATCH] Extending getrusage

[Claudio Scordino]

> > I'd be reluctant to support this change without a compelling description of
> > why we actually want it.
>
> It offers nothing that isn't available elsewhere (I think), except more
> cheaply. It also has the potential to be multiplatform one day. Right now to
> get the CPU usage of a random pid, one has to implement the equivalent of
> /proc/ parsing for each platform.
>
> If at least Linux did 'getrusage for foreign pid', that would clean up
> things up somewhat already.
>
> It might even mean a 'portable top', and who knows, more unixes might
> follow.
>
> I for one would save work having a getrusage that does this.
>
>         Bert

Exactly! My modification was meant to add the possibility of having
usage information about another process at user level.

Recently, while writing some code at user level, I needed a fast way
to have such information about another process.
I wanted to use the getrusage syscall, then I found out that the
current implementation does not allow to get information about another
process. Therefore, I proposed my patch.

As Bert said, it offers nothing that isn't available elsewhere, but
more cheaply.

This patch with the changes that you asked me follows.

Thanks,

                Claudio

Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com>
diff -uprN linux-2.6.16.9/kernel/sys.c linux-2.6.16.9-new/kernel/sys.c
--- linux-2.6.16.9/kernel/sys.c	2006-04-19 02:10:14.000000000 -0400
+++ linux-2.6.16.9-new/kernel/sys.c	2006-04-21 05:53:16.000000000 -0400
@@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
 	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
 }

+/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
 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) {
+		if (who <= 0)
+			goto bad;
+		tsk = find_task_by_pid(who);
+		if (tsk = NULL)
+			goto bad;
+		if ((tsk != current) && security_ptrace(current, tsk))
+			goto bad;
+		/* current can get info about tsk */
+		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)

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] Extending getrusage

[Claudio Scordino]

> > I'd be reluctant to support this change without a compelling description of
> > why we actually want it.
>
> It offers nothing that isn't available elsewhere (I think), except more
> cheaply. It also has the potential to be multiplatform one day. Right now to
> get the CPU usage of a random pid, one has to implement the equivalent of
> /proc/ parsing for each platform.
>
> If at least Linux did 'getrusage for foreign pid', that would clean up
> things up somewhat already.
>
> It might even mean a 'portable top', and who knows, more unixes might
> follow.
>
> I for one would save work having a getrusage that does this.
>
>         Bert

Exactly! My modification was meant to add the possibility of having
usage information about another process at user level.

Recently, while writing some code at user level, I needed a fast way
to have such information about another process.
I wanted to use the getrusage syscall, then I found out that the
current implementation does not allow to get information about another
process. Therefore, I proposed my patch.

As Bert said, it offers nothing that isn't available elsewhere, but
more cheaply.

This patch with the changes that you asked me follows.

Thanks,

                Claudio

Signed-off-by: Claudio Scordino <cloud.of.andor@gmail.com>
diff -uprN linux-2.6.16.9/kernel/sys.c linux-2.6.16.9-new/kernel/sys.c
--- linux-2.6.16.9/kernel/sys.c	2006-04-19 02:10:14.000000000 -0400
+++ linux-2.6.16.9-new/kernel/sys.c	2006-04-21 05:53:16.000000000 -0400
@@ -1765,11 +1765,30 @@ int getrusage(struct task_struct *p, int
 	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
 }

+/* who can be RUSAGE_SELF, RUSAGE_CHILDREN or a valid pid */
 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) {
+		if (who <= 0)
+			goto bad;
+		tsk = find_task_by_pid(who);
+		if (tsk == NULL)
+			goto bad;
+		if ((tsk != current) && security_ptrace(current, tsk))
+			goto bad;
+		/* current can get info about tsk */
+		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)

[KJ] Re: [PATCH] Extending getrusage

[Ulrich Drepper]

On 4/21/06, Claudio Scordino <cloud.of.andor@gmail.com> wrote:
> Recently, while writing some code at user level, I needed a fast way
> to have such information about another process.

That's not very specific.  And one program isn't really a compelling
reason.  You should specify with some level of detail why you need
that information and why you cannot depend on collaboration of the
process you try to get the information for.


> -               return -EINVAL;
> -       return getrusage(current, who, ru);
> +       struct rusage r;
> +       struct task_struct *tsk = current;
> +       read_lock(&tasklist_lock);

You are introducing scalability problems where there were none before.
 Even if there is some justification revealed in the end IMO the patch
shouldn't be accepted in this form.  You should not slow down the
normal case of operation.  If the current thread is observed the lock
isn't needed.

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] Extending getrusage

[Ulrich Drepper]

On 4/21/06, Claudio Scordino <cloud.of.andor@gmail.com> wrote:
> Recently, while writing some code at user level, I needed a fast way
> to have such information about another process.

That's not very specific.  And one program isn't really a compelling
reason.  You should specify with some level of detail why you need
that information and why you cannot depend on collaboration of the
process you try to get the information for.


> -               return -EINVAL;
> -       return getrusage(current, who, ru);
> +       struct rusage r;
> +       struct task_struct *tsk = current;
> +       read_lock(&tasklist_lock);

You are introducing scalability problems where there were none before.
 Even if there is some justification revealed in the end IMO the patch
shouldn't be accepted in this form.  You should not slow down the
normal case of operation.  If the current thread is observed the lock
isn't needed.

Re: [PATCH] Extending getrusage

[Claudio Scordino]

On Friday 21 April 2006 16:32, Ulrich Drepper wrote:
> On 4/21/06, Claudio Scordino <cloud.of.andor@gmail.com> wrote:
> > Recently, while writing some code at user level, I needed a fast way
> > to have such information about another process.
> 
> That's not very specific.  And one program isn't really a compelling
> reason.  You should specify with some level of detail why you need
> that information and why you cannot depend on collaboration of the
> process you try to get the information for.

I'm just adding a new feature "for free":

- Without adding any new syscall
- Without changing the prototype of the current syscall (sys_getrusage)
- Ensuring backward compatibility (all old programs will continue to work properly)

This new feature can be used by any process that needs to profile another process for whatever reason.

Since it just adds a new feature without any drawback, I don't understand why it would be wrong...
 
> 
> > -               return -EINVAL;
> > -       return getrusage(current, who, ru);
> > +       struct rusage r;
> > +       struct task_struct *tsk = current;
> > +       read_lock(&tasklist_lock);
> 
> You are introducing scalability problems where there were none before.
>  Even if there is some justification revealed in the end IMO the patch
> shouldn't be accepted in this form.  You should not slow down the
> normal case of operation.  If the current thread is observed the lock
> isn't needed.

I think you're wrong: I'm not introducing any *new* scalability issue. 

Currently the code is:

asmlinkage long sys_getrusage(int who, struct rusage __user *ru)
{
        if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN)
                return -EINVAL;
        return getrusage(current, who, ru);
}

which in turn calls

int getrusage(struct task_struct *p, int who, struct rusage __user *ru)
{
        struct rusage r;
        read_lock(&tasklist_lock);
        k_getrusage(p, who, &r);
        read_unlock(&tasklist_lock);
        return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
}

As you can see, the lock is acquired also when not needed. That's how it is currently implemented.

With my patch, k_getrusage is called directly from sys_getrusage without going through getrusage.

               Claudio