Skip to contentSkip to conversation

Read THREAD_CPUTIME clock from other processes.

PATCH In review Dario Faggioli 6 participants 23 messages Dec 23, 2010 – Jan 8, 2011

lore
Share & tools
lore

Dario Faggioli

author
#0 lore raw

PATCH 1/1 Read THREAD_CPUTIME clock from other processes.

Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside the process that spawned it with this code:

        if (clock_getcpuclockid(tid, &clockid) != 0) {
                perror("clock_getcpuclockid");
                exit(EXIT_FAILURE);
        }

results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

OTH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid.

This is because clock_getcpuclockid forbids accessing thread specific CPU-time clocks from outside the thread group. This is not requested (e.g., by POSIX) to be like this, or at least no indication that such operation should fail can be found in `man clock_getcpuclockid' and alike.

However, having such capability could be useful, if you want to monitor the execution of a bunch of thread from some kind of "manager" which might not be part of the same process. A typical example that could benefit from this could be the JACK graph-manager.

Therefore, this patch removes such limitation and enables the following behaviour, for the threaded and process-based case, respectively:

### Testing tid 24704: CPU-time clock for PID 24704 is 1.049570008 seconds ### Testing tid 24706: CPU-time clock for PID 24706 is 1.028650801seconds

### Testing tid 24715: CPU-time clock for PID 24715 is 0.000957685 seconds ### Testing tid 24717: CPU-time clock for PID 24717 is 1.045351509 seconds

Signed-off-by Dario Faggioli <[email protected]>
---
 kernel/posix-cpu-timers.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)
kernel/posix-cpu-timers.c +12 -11
index 05bb717..b0ed8cf 100644
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t which_clock)
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p)
 		error = -EINVAL;
-	}
 	rcu_read_unlock();
 
 	return error;

@@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+
+			if (CPUCLOCK_PERTHREAD(which_clock) &&
+			    same_thread_group(p, current)) {
+				error = cpu_clock_sample(which_clock,
+							 p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
-				if (thread_group_leader(p) && p->sighand) {
+				if (!CPUCLOCK_PERTHREAD(which_clock) &&
+				    thread_group_leader(p) && p->sighand)
 					error =
 					    cpu_clock_sample_group(which_clock,
-							           p, &rtn);
-				}
+								   p, &rtn);
+				else
+					error = cpu_clock_sample(which_clock,
+								 p, &rtn);
 				read_unlock(&tasklist_lock);
 			}
 		}
Signature · 9 lines
1.7.2.3


-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#1 lore raw
Quoted 25 lines · Dario Faggioli

Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside the process that spawned it with this code:

        if (clock_getcpuclockid(tid, &clockid) != 0) {
                perror("clock_getcpuclockid");
                exit(EXIT_FAILURE);
        }

results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

OTH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid.

This is because clock_getcpuclockid forbids accessing thread specific CPU-time clocks from outside the thread group. This is not requested (e.g., by POSIX) to be like this, or at least no indication that such operation should fail can be found in `man clock_getcpuclockid' and alike.

Quoted 8 lines

However, having such capability could be useful, if you want to monitor the execution of a bunch of thread from some kind of "manager" which might not be part of the same process. A typical example that could benefit from this could be the JACK graph-manager.

Therefore, this patch removes such limitation and enables the following behaviour, for the threaded and process-based case, respectively:

Can't comment, I never understood this.

A couple of nits on the patch itself,

kernel/posix-cpu-timers.c static int check_clock(const clockid_t which_clock) · line 45

Quoted 11 lines
kernel/posix-cpu-timers.c +1 -3
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t which_clock)
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p)
 		error = -EINVAL;
-	}

This changes the behaviour of sys_clock_settime(). Probably doesn't matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD && !group_leader should result in -EINAVL as before.

Quoted 18 lines
kernel/posix-cpu-timers.c +6 -6
@@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+
+			if (CPUCLOCK_PERTHREAD(which_clock) &&
+			    same_thread_group(p, current)) {
+				error = cpu_clock_sample(which_clock,
+							 p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
-				if (thread_group_leader(p) && p->sighand) {
+				if (!CPUCLOCK_PERTHREAD(which_clock) &&

kernel/posix-cpu-timers.c int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp) · line 365

Quoted 10 lines
kernel/posix-cpu-timers.c +5 -2
+				    thread_group_leader(p) && p->sighand)
 					error =
 					    cpu_clock_sample_group(which_clock,
-							           p, &rtn);
-				}
+								   p, &rtn);
+				else
+					error = cpu_clock_sample(which_clock,
+								 p, &rtn);
 				read_unlock(&tasklist_lock);

Can't understand... why did you duplicate cpu_clock_sample() ?

IOW, it seems to me you could simply kill the
"if (same_thread_group(p, current)) {" line with the same efect, no?

Oleg.

author
#2 lore raw
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines

Therefore, this patch removes such limitation and enables the following behaviour, for the threaded and process-based case, respectively:

Can't comment, I never understood this.

If I can ask... What's that you never understood? Why the limitation is there? Or something else?

Quoted 12 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t which_clock) · line 45

Quoted 7 lines
kernel/posix-cpu-timers.c +1 -3
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p)
 		error = -EINVAL;
-	}

This changes the behaviour of sys_clock_settime(). Probably doesn't matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD && !group_leader should result in -EINAVL as before.

Oops, sure, you're right, I can fix this. :-)

Quoted 31 lines
Quoted 18 lines
kernel/posix-cpu-timers.c +6 -6
@@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+
+			if (CPUCLOCK_PERTHREAD(which_clock) &&
+			    same_thread_group(p, current)) {
+				error = cpu_clock_sample(which_clock,
+							 p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
-				if (thread_group_leader(p) && p->sighand) {
+				if (!CPUCLOCK_PERTHREAD(which_clock) &&

kernel/posix-cpu-timers.c int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp) · line 365

Quoted 10 lines
kernel/posix-cpu-timers.c +5 -2
+				    thread_group_leader(p) && p->sighand)
 					error =
 					    cpu_clock_sample_group(which_clock,
-							           p, &rtn);
-				}
+								   p, &rtn);
+				else
+					error = cpu_clock_sample(which_clock,
+								 p, &rtn);
 				read_unlock(&tasklist_lock);

Can't understand... why did you duplicate cpu_clock_sample() ?

Quoted 3 lines

IOW, it seems to me you could simply kill the
"if (same_thread_group(p, current)) {" line with the same efect, no?

Well, yes, but looking at the original code I thought that in the ! same_thread_group() case I might need the tasklist_lock...

Am I wrong? Is it there just because of cpu_clock_sample_group()?

Thanks and Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#3 lore raw
Quoted 10 lines · Dario Faggioli
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines

Therefore, this patch removes such limitation and enables the following behaviour, for the threaded and process-based case, respectively:

Can't comment, I never understood this.

If I can ask... What's that you never understood? Why the limitation is there?

Yes. IOW, I agree it looks strange, clock_gettime() can sample the whole group but not a single thread.

Quoted 31 lines
Quoted 31 lines
Quoted 18 lines
kernel/posix-cpu-timers.c +6 -6
@@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+
+			if (CPUCLOCK_PERTHREAD(which_clock) &&
+			    same_thread_group(p, current)) {
+				error = cpu_clock_sample(which_clock,
+							 p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
-				if (thread_group_leader(p) && p->sighand) {
+				if (!CPUCLOCK_PERTHREAD(which_clock) &&

kernel/posix-cpu-timers.c int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp) · line 365

Quoted 10 lines
kernel/posix-cpu-timers.c +5 -2
+				    thread_group_leader(p) && p->sighand)
 					error =
 					    cpu_clock_sample_group(which_clock,
-							           p, &rtn);
-				}
+								   p, &rtn);
+				else
+					error = cpu_clock_sample(which_clock,
+								 p, &rtn);
 				read_unlock(&tasklist_lock);

Can't understand... why did you duplicate cpu_clock_sample() ?

Quoted 7 lines
Quoted 3 lines

IOW, it seems to me you could simply kill the
"if (same_thread_group(p, current)) {" line with the same efect, no?

Well, yes, but looking at the original code I thought that in the ! same_thread_group() case I might need the tasklist_lock...

Am I wrong? Is it there just because of cpu_clock_sample_group()?

Yes, it is because of _group (we are going to sample all sub-threads), not because of !same_thread_group().

Oh. In fact we should remove this tasklist, but this is another story.

Oleg.

author
#4 lore raw
In reply to Dario Faggioli
Quoted 12 lines · Dario Faggioli
Quoted 12 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t which_clock) · line 45

Quoted 7 lines
kernel/posix-cpu-timers.c +1 -3
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p)
 		error = -EINVAL;
-	}

This changes the behaviour of sys_clock_settime(). Probably doesn't matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD && !group_leader should result in -EINAVL as before.

Quoted 2 lines

Oops, sure, you're right, I can fix this. :-)

Mmm... This is trickier than I remembered expected. Apparently, glibc always gives us a !CPUCLOCK_PERTHREAD() clockid, since it uses MAKE_PROCESS_CLOCK() when clock_cpugetclockid is called.

Therefore, the take two of this thing will be a little different from this first posting...

Thanks and Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#5 lore raw
In reply to Dario Faggioli
Quoted 12 lines · On Thu, 23 Dec 2010 17:21:43 +0100 Dario Faggioli wrote:

Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside the process that spawned it with this code:

        if (clock_getcpuclockid(tid, &clockid) != 0) {
                perror("clock_getcpuclockid");
                exit(EXIT_FAILURE);
        }

results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

Quoted 5 lines

OTH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid.

"DNS service for this domain has expired with DNS Made Easy" :(

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
desserts: http://www.xenotime.net/linux/recipes/

author
#6 lore raw
Quoted 14 lines · Randy Dunlap
Quoted 12 lines · On Thu, 23 Dec 2010 17:21:43 +0100 Dario Faggioli wrote:

Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside the process that spawned it with this code:

        if (clock_getcpuclockid(tid, &clockid) != 0) {
                perror("clock_getcpuclockid");
                exit(EXIT_FAILURE);
        }

results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

Quoted 9 lines
Quoted 5 lines

OTH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid.

"DNS service for this domain has expired with DNS Made Easy" :(

Yep, I saw that... It seems gitorious is having some sort of big big problem! :-O

I've to run now, so, here's a really quick-&-dirty tar of the repo...

Thanks and Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
author
#7 lore raw

PATCH RESEND 1/1 Reading POSIX CPU timer from outside the process.

In reply to Dario Faggioli

Trying to call clock_getcpuclockid (and then clock_gettime) on a thread from outside the process that spawned it results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

OTOH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid

All that because clock_getcpuclockid forbids accessing thread specific CPU-time clocks from outside the thread group, but this is not requested (e.g., by POSIX), or at least no indication that it should fail can be found in `man clock_getcpuclockid' and alike.
Moreover, having such capability could be useful, if you want to monitor the execution of a bunch of thread from some kind of "manager" which might not be part of the same process. A typical example is the JACK graph-manager.

Therefore, this commit makes clock_getcpuclockid behave as follows:
- if it's called on a tid which is also a PID (i.e., the thread is a thread group leader), it returns the CLOCK_PROCESS_CPUTIME_ID of the process;
- if it's called on a tid of a non-group leader, it returns the CLOCK_THREAD_CPUTIME_ID of such specific thread.

This enables the following behaviour, for the threaded and process-based scenarios, respectively:

### Testing tid 24704: CPU-time clock for PID 24704 is 1.049570008 seconds ### Testing tid 24706: CPU-time clock for PID 24706 is 1.028650801 seconds

### Testing tid 24715: CPU-time clock for PID 24715 is 0.000957685 seconds ### Testing tid 24717: CPU-time clock for PID 24717 is 1.045351509 seconds

Signed-off-by Dario Faggioli <[email protected]>
---
 kernel/posix-cpu-timers.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)
kernel/posix-cpu-timers.c +5 -8
index 05bb717..ddbbabc 100644
@@ -39,10 +39,9 @@ static int check_clock(const clockid_t which_clock)
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
+		   same_thread_group(p, current) && !has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();
 
 	return error;

@@ -349,11 +348,9 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+			if (CPUCLOCK_PERTHREAD(which_clock) ||
+			    !thread_group_leader(p)) {
+				error = cpu_clock_sample(which_clock, p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
 				if (thread_group_leader(p) && p->sighand) {
Signature · 9 lines
1.7.2.3


-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#8 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

This patch doesn't look right,

Quoted 6 lines · Dario Faggioli

Trying to call clock_getcpuclockid (and then clock_gettime) on a thread from outside the process that spawned it results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds ### Testing tid 24209: clock_getcpuclockid: Success

Quoted 8 lines

OTOH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid

All that because clock_getcpuclockid forbids accessing thread specific CPU-time clocks from outside the thread group,

First of all, linux has no clock_getcpuclockid() system call, so the changelog looks confusing.

glibc implements clock_getcpuclockid() via clock_getres(), that is why the change in check_clock() can help.

Quoted 6 lines

Therefore, this commit makes clock_getcpuclockid behave as follows:
- if it's called on a tid which is also a PID (i.e., the thread is a thread group leader), it returns the CLOCK_PROCESS_CPUTIME_ID of the process;
- if it's called on a tid of a non-group leader, it returns the CLOCK_THREAD_CPUTIME_ID of such specific thread.

And both changes look wrong to me.

kernel/posix-cpu-timers.c static int check_clock(const clockid_t which_clock) · line 45

Quoted 13 lines
kernel/posix-cpu-timers.c +2 -3
@@ -39,10 +39,9 @@ static int check_clock(const clockid_t which_clock)
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
+		   same_thread_group(p, current) && !has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

How so? For example, with this change clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?

kernel/posix-cpu-timers.c int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp) · line 353

Quoted 12 lines
kernel/posix-cpu-timers.c +3 -5
@@ -349,11 +348,9 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
 		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (p) {
-			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+			if (CPUCLOCK_PERTHREAD(which_clock) ||
+			    !thread_group_leader(p)) {
+				error = cpu_clock_sample(which_clock, p, &rtn);

IOW, we ignore CPUCLOCK_PERTHREAD() if !thread_group_leader(), this can't be right.

I think, if we want to remove this limitation, we need something like the patch below. If it doesn't help, we should fix glibc.

Oleg.

kernel/posix-cpu-timers.c +2 -7
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t w
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) || has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();
 
 	return error;

@@ -350,10 +348,7 @@ int posix_cpu_clock_get(const clockid_t 
 		p = find_task_by_vpid(pid);
 		if (p) {
 			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+				error = cpu_clock_sample(which_clock, p, &rtn);
 			} else {
 				read_lock(&tasklist_lock);
 				if (thread_group_leader(p) && p->sighand) {
author
#9 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 2 lines · Oleg Nesterov

This patch doesn't look right,

Sorry then... :-(

Quoted 6 lines
Quoted 2 lines

All that because clock_getcpuclockid forbids accessing thread specific CPU-time clocks from outside the thread group,

First of all, linux has no clock_getcpuclockid() system call, so the changelog looks confusing.

Sure, you're right, this could have been more clear.

Quoted 4 lines
Quoted 4 lines
kernel/posix-cpu-timers.c +0 -2
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
Quoted 9 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 44

Quoted 5 lines
kernel/posix-cpu-timers.c +2 -1
+	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
+		   same_thread_group(p, current) && !has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

How so? For example, with this change clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?

I tested all the clock_getres() calls that came to my mind (at least the one that are possible from an userspace program), and they always worked because of this (still in check_clock):

        const pid_t pid = CPUCLOCK_PID(which_clock);

        if (pid == 0)
                return 0;

Which triggers all the times, except when you actually try to get a CPU clockid from outside the process, but that's not possible with getres.

Anyway, looking at the code again I agree, it may work, but it's not something I really like! :-|

The whole point was about, given the current implementation of clock_getcpuclockid done by glibc, can we remove that "failed with success" (showed in the changelog) thing and come up with some meaningful clockid for that situation? It's more than possible for the answer to be no!!! :-P

Quoted 6 lines

I think, if we want to remove this limitation, we need something like the patch below. If it doesn't help, we should fix glibc.

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 45

Quoted 10 lines
kernel/posix-cpu-timers.c +1 -3
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t w
 
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) || has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false in this case.

kernel/posix-cpu-timers.c int posix_cpu_clock_get(const clockid_t · line 351

Quoted 10 lines
kernel/posix-cpu-timers.c +1 -4
 	return error;

@@ -350,10 +348,7 @@ int posix_cpu_clock_get(const clockid_t 
 		p = find_task_by_vpid(pid);
 		if (p) {
 			if (CPUCLOCK_PERTHREAD(which_clock)) {
-				if (same_thread_group(p, current)) {
-					error = cpu_clock_sample(which_clock,
-								 p, &rtn);
-				}
+				error = cpu_clock_sample(which_clock, p, &rtn);

Same as above... To the point that I'm now wondering if we ever take this branch here...

BTW, again, I see your point, the fix might need to happen at glibc level. I'll check that and come back if I find something interesting.

Thanks anyway,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#10 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 20 lines · Dario Faggioli
Quoted 4 lines · Oleg Nesterov
Quoted 4 lines
kernel/posix-cpu-timers.c +0 -2
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
Quoted 9 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 44

Quoted 5 lines
kernel/posix-cpu-timers.c +2 -1
+	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
+		   same_thread_group(p, current) && !has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

How so? For example, with this change clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?

I tested all the clock_getres() calls that came to my mind (at least the one that are possible from an userspace program), and they always worked because of this (still in check_clock):

Quoted 6 lines
        const pid_t pid = CPUCLOCK_PID(which_clock);

        if (pid == 0)
                return 0;

Which triggers all the times,

No, please note pid_of_sub_thread above.

Quoted 5 lines

The whole point was about, given the current implementation of clock_getcpuclockid done by glibc, can we remove that "failed with success" (showed in the changelog) thing and come up with some meaningful clockid for that situation? It's more than possible for the answer to be no!!! :-P

I think we should change glibc if clock_getcpuclockid() doesn't work, please see below.

Quoted 7 lines
Quoted 7 lines

I think, if we want to remove this limitation, we need something like the patch below. If it doesn't help, we should fix glibc.

kernel/posix-cpu-timers.c +0 -0
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t w
Quoted 11 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 45

Quoted 9 lines
kernel/posix-cpu-timers.c +1 -3
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) || has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false in this case.

I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right? But we shouldn't add the hacks to the kernel to hide the limitations in glibc.

Quoted 2 lines

BTW, again, I see your point, the fix might need to happen at glibc level. I'll check that and come back if I find something interesting.

Yes, please.

BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
I guess it is clockid.c...

You do not need clock_getcpuclockid() at all. In fact I do not really understand what this helper should actually do, probably it is only needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK() to sample a single thread via clock_gettime().

IOW. Unless I missed something, with this patch, the only problem is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(), I do not think this is the kernel problem.

Oleg.

author
#11 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 7 lines · Oleg Nesterov
Quoted 7 lines
Quoted 3 lines

How so? For example, with this change clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?

I tested all the clock_getres() calls that came to my mind (at least the one that are possible from an userspace program), and they always worked because of this (still in check_clock):

Quoted 9 lines
Quoted 6 lines
        const pid_t pid = CPUCLOCK_PID(which_clock);

        if (pid == 0)
                return 0;

Which triggers all the times,

No, please note pid_of_sub_thread above.

Sure, I saw that... I was referring to all the clock_getres() and clock_{get,set}time() I was able to call without using directly MAKE_THREAD_CPUCLOCK.

Quoted 7 lines
Quoted 2 lines

Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false in this case.

I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right? But we shouldn't add the hacks to the kernel to hide the limitations in glibc.

Exactly. Actually, I didn't noticed this too when I first started with this patch, and the fact that my first version worked -- which was by chance, I can say it now :-P -- made me think it was worthwhile to bend the semantic a bit, to enable this new capability... But making it work is hackish, I see it now. Sorry. :-)

Quoted 3 lines

BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
I guess it is clockid.c...

Yep. A very trivial one, I agree, but it's just to show the issue.

Quoted 5 lines

You do not need clock_getcpuclockid() at all. In fact I do not really understand what this helper should actually do, probably it is only needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK() to sample a single thread via clock_gettime().

Fine, but, is that macro available for an application developer? Because I can find it in kernel and glibc sources, but not in my /usr/include/*, which is the motivation behind this attempt... But it might be my fault! :-P

Quoted 4 lines

IOW. Unless I missed something, with this patch, the only problem is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(), I do not think this is the kernel problem.

Agreed, sorry for wasting (hopefully not too much) people's time. :-(

Thanks and Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#12 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 8 lines · Dario Faggioli
Quoted 5 lines · Oleg Nesterov

You do not need clock_getcpuclockid() at all. In fact I do not really understand what this helper should actually do, probably it is only needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK() to sample a single thread via clock_gettime().

Quoted 4 lines

Fine, but, is that macro available for an application developer? Because I can find it in kernel and glibc sources, but not in my /usr/include/*, which is the motivation behind this attempt... But it might be my fault! :-P

Yes, I do not see MAKE_*_CPUCLOCK() in /usr/include.

Quoted 5 lines
Quoted 4 lines

IOW. Unless I missed something, with this patch, the only problem is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(), I do not think this is the kernel problem.

Agreed, sorry for wasting (hopefully not too much) people's time. :-(

No, I think you have a point. I'd suggest you to re-send the patch which removes this limitation from kernel side.

My only objection was, we shouldn't add the hacks to overcome the limitations in glibc. Say, posix_cpu_clock_get() should only check CPUCLOCK_PERTHREAD(), it should not treat !group_leader specially just because getcpuclockid() can construct the proper clock id. This should be solved in userland.

Oleg.

#13 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

In reply to Oleg Nesterov
Quoted 30 lines · Oleg Nesterov
Quoted 20 lines · Dario Faggioli
Quoted 4 lines · Oleg Nesterov
Quoted 4 lines
kernel/posix-cpu-timers.c +0 -2
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
Quoted 9 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 44

Quoted 5 lines
kernel/posix-cpu-timers.c +2 -1
+	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
+		   same_thread_group(p, current) && !has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

How so? For example, with this change clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?

I tested all the clock_getres() calls that came to my mind (at least the one that are possible from an userspace program), and they always worked because of this (still in check_clock):

Quoted 6 lines
        const pid_t pid = CPUCLOCK_PID(which_clock);

        if (pid == 0)
                return 0;

Which triggers all the times,

No, please note pid_of_sub_thread above.

Quoted 8 lines
Quoted 5 lines

The whole point was about, given the current implementation of clock_getcpuclockid done by glibc, can we remove that "failed with success" (showed in the changelog) thing and come up with some meaningful clockid for that situation? It's more than possible for the answer to be no!!! :-P

I think we should change glibc if clock_getcpuclockid() doesn't work, please see below.

http://www.kernel.org/doc/man-pages/online/pages/man3/pthread_getcpuclockid.3.html

this one works.
ok... it takes a pthread_t for identifying the thread. but it works.

Quoted 44 lines
Quoted 8 lines
Quoted 8 lines

I think, if we want to remove this limitation, we need something like the patch below. If it doesn't help, we should fix glibc.

kernel/posix-cpu-timers.c +0 -0
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t w
Quoted 11 lines

kernel/posix-cpu-timers.c static int check_clock(const clockid_t w · line 45

Quoted 9 lines
kernel/posix-cpu-timers.c +1 -3
 	rcu_read_lock();
 	p = find_task_by_vpid(pid);
-	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-		   same_thread_group(p, current) : has_group_leader_pid(p))) {
+	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) || has_group_leader_pid(p)))
 		error = -EINVAL;
-	}
 	rcu_read_unlock();

Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false in this case.

I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right? But we shouldn't add the hacks to the kernel to hide the limitations in glibc.

Quoted 2 lines

BTW, again, I see your point, the fix might need to happen at glibc level. I'll check that and come back if I find something interesting.

Yes, please.

BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
I guess it is clockid.c...

You do not need clock_getcpuclockid() at all. In fact I do not really understand what this helper should actually do, probably it is only needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK() to sample a single thread via clock_gettime().

IOW. Unless I missed something, with this patch, the only problem is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(), I do not think this is the kernel problem.

Oleg.

Signature · 1 line
torben Hohn
author
#14 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 9 lines · torbenh
Quoted 2 lines

I think we should change glibc if clock_getcpuclockid() doesn't work, please see below.

http://www.kernel.org/doc/man-pages/online/pages/man3/pthread_getcpuclockid.3.html

this one works.
ok... it takes a pthread_t for identifying the thread. but it works.

Well, I knew this, I knew it very well, as you can see from the code on the git I pointed to (it's part of one of the examples!).

the thing is that I believe there are situations where it could be useful to sample CLOCK_THREAD_CPUTIME_ID from a _different_ process, and in that case, you can't access that thread's pthread-id, can you? :-O

The jack2 code I saw could be one of these "potential user", and AFAICT your jack1 might be another one, no?
Yeah, I know, whether you can/want use this or not also depends on other issues, but you are in a _different_ process --and thus you can't use pthread_* calls-- aren't you?

I'm now looking at Oleg's solution and into glibc as well... I'll come out with something ASAP.

Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
#15 lore raw

Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

Quoted 17 lines · Dario Faggioli
Quoted 9 lines · torbenh
Quoted 2 lines

I think we should change glibc if clock_getcpuclockid() doesn't work, please see below.

http://www.kernel.org/doc/man-pages/online/pages/man3/pthread_getcpuclockid.3.html

this one works.
ok... it takes a pthread_t for identifying the thread. but it works.

Well, I knew this, I knew it very well, as you can see from the code on the git I pointed to (it's part of one of the examples!).

the thing is that I believe there are situations where it could be useful to sample CLOCK_THREAD_CPUTIME_ID from a _different_ process, and in that case, you can't access that thread's pthread-id, can you? :-O

Quoted 5 lines

The jack2 code I saw could be one of these "potential user", and AFAICT your jack1 might be another one, no?
Yeah, I know, whether you can/want use this or not also depends on other issues, but you are in a _different_ process --and thus you can't use pthread_* calls-- aren't you?

i would need to tell the tid to jackd anyways.
i could as well just communicate the clock_id.

getting the clock_id into the other process is not a problem. the problem is that i am not allowed to read out the clock.

Quoted 13 lines

I'm now looking at Oleg's solution and into glibc as well... I'll come out with something ASAP.

Regards,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]
Signature · 1 line
torben Hohn
#16 lore raw
In reply to Dario Faggioli

clock_getcpuclockid is the POSIX interface for using a process-wide CPU clock. pthread_getcpuclockid is the POSIX interface for using a thread-specific CPU clock. There is no POSIX interface for using the thread-specific clock of a thread in a different process because POSIX does not have the notion of global identification of threads at all. The very idea that you could know anything about an individual thread in a different process is a Linuxism. If you want to do something like that, then there is no reason to use the POSIX standard interfaces rather than just using the Linux-specific clockid_t generation macros in the first place.

When the CPU clock interfaces were introduced to the kernel, it was considered a potential security issue (information leak) to be able to access the thread clocks of another process, because there had never been a way for one process to access such information from another process before. We took the conservative route of permitting it only within the same process.

This can certainly be enhanced, but it opens some cans of worms about the security question. It is probably still considered an unsafe information leak to let every process examine every other process's thread clocks. I'll leave that judgement to security folks. The intermediate route of conversatism is to allow it only for processes owned by the same user, which has some complexities with races between UID changes and clock/timer calls.

As well as the information leak, it is most certainly a DoS attack vector to allow one process to set CPU timers an another process or its threads. Setting timers causes the timed thread itself to do work proportional to the number of timers set.

Thanks,
Roland

#17 lore raw
Quoted 4 lines · Roland McGrath

This can certainly be enhanced, but it opens some cans of worms about the security question. It is probably still considered an unsafe information leak to let every process examine every other process's thread clocks.

Yes, I was worried about possible security issues too. But, it seems, /proc/pid/task/tid/stat (do_task_stat) shows ->utime/stime anyway. And /proc/schedstat shows sum_exec_runtime.

Quoted 1 line

I'll leave that judgement to security folks.

Agreed.

Quoted 2 lines

As well as the information leak, it is most certainly a DoS attack vector to allow one process to set CPU timers an another process or its threads.

No, the suggested change doesn't go that far, afaics. It only modifies
check_clock: this affects clock_getres and clock_set (which does nothing), and posix_cpu_clock_get: affects clock_gettime().

Oleg.

#18 lore raw
Quoted 2 lines

Yes, I was worried about possible security issues too. But, it seems, /proc/pid/task/tid/stat (do_task_stat) shows ->utime/stime anyway.

This is constrained by ptrace_may_access(task, PTRACE_MODE_READ).

Quoted 1 line

And /proc/schedstat shows sum_exec_runtime.

This probably should be constrained in exactly the same ways as .../stat is.

I think the constraints on /proc/.../stat reflect the most considered judgement of the security folks. I suspect that the lack of constraint on /proc/.../schedstat reflects the scheduler folks just having failed to consider the same issues. IMHO all access to the equivalent kinds of information should have the same security constraints.

Quoted 3 lines

No, the suggested change doesn't go that far, afaics. It only modifies
check_clock: this affects clock_getres and clock_set (which does nothing), and posix_cpu_clock_get: affects clock_gettime().

That's good, but it should be mentioned so that next year someone doesn't come back with a "there's no reason you shouldn't be able to" and go unchallenged.

Thanks,
Roland

#19 lore raw
Quoted 5 lines · Roland McGrath

I think the constraints on /proc/.../stat reflect the most considered judgement of the security folks. I suspect that the lack of constraint on /proc/.../schedstat reflects the scheduler folks just having failed to consider the same issues. IMHO all access to the equivalent kinds of information should have the same security constraints.

FWIW I'm fine with adding a PTRACE_MAY_READ constraint on schedstat.

#20 lore raw
In reply to Roland McGrath
Quoted 5 lines · Roland McGrath
Quoted 3 lines

Yes, I was worried about possible security issues too. But, it seems, /proc/pid/task/tid/stat (do_task_stat) shows ->utime/stime anyway.

This is constrained by ptrace_may_access(task, PTRACE_MODE_READ).

Well, ptrace_may_access() protects only get_wchan/eip/esp, but not utime/stime.

Quoted 3 lines

That's good, but it should be mentioned so that next year someone doesn't come back with a "there's no reason you shouldn't be able to" and go unchallenged.

Yes, agreed. The changelog should be improved.

Oleg.

#21 lore raw
Quoted 2 lines

Well, ptrace_may_access() protects only get_wchan/eip/esp, but not utime/stime.

Oh, sorry. I misread the code. As I said, I leave it to the security folks to decide what the appropriate constraints are. All the mechanisms for accessing the same information should just be consistent. So perhaps this is indeed fine.

Thanks,
Roland

author
#22 lore raw
In reply to Roland McGrath
Quoted 6 lines · Roland McGrath

clock_getcpuclockid is the POSIX interface for using a process-wide CPU clock. pthread_getcpuclockid is the POSIX interface for using a thread-specific CPU clock. There is no POSIX interface for using the thread-specific clock of a thread in a different process because POSIX does not have the notion of global identification of threads at all.

Sure, even just the fact that the tid is involved makes it unequivocally a Linux extension to such interface.

Quoted 6 lines

The very
idea that you could know anything about an individual thread in a different process is a Linuxism. If you want to do something like that, then there is no reason to use the POSIX standard interfaces rather than just using the Linux-specific clockid_t generation macros in the first place.

And I'm perfectly fine with this, if there's a consensus around it! :-)

The whole point is that this is not possible right now, since the clockid_t generators are not accessible from userspace... Should I go for this?

Quoted 7 lines

When the CPU clock interfaces were introduced to the kernel, it was considered a potential security issue (information leak) to be able to access the thread clocks of another process, because there had never been a way for one process to access such information from another process before. We took the conservative route of permitting it only within the same process.

That crossed my mind as well (I think I mentioned at least in one of the e-mail if not in the changelog). Then I thought that if it's possible to access an arbitrary process' CPU timer, why it shouldn't be possible to access the one of an arbitrary thread... But, as you, I'm not a "security guy", and I would be the first one to suggest to drop this if it is considered a security risk! :-)

Quoted 5 lines

As well as the information leak, it is most certainly a DoS attack vector to allow one process to set CPU timers an another process or its threads. Setting timers causes the timed thread itself to do work proportional to the number of timers set.

Yep, but as said, no room for timer setting, neither with or without the patch, due to the nature of the clock itlsef.

Thanks,
Dario

Signature · 5 lines
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- [email protected]