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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1293572304.2899.1214.camel@Palantir>
Date:	Tue, 28 Dec 2010 22:38:24 +0100
From:	Dario Faggioli <raistlin@...ux.it>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	torbenh <torbenh@....de>, john.stultz@...aro.org,
	roland@...hat.com, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Stanislaw Gruszka <sgruszka@...hat.com>,
	Dhaval Giani <dhaval.giani@...il.com>,
	Randy Dunlap <rdunlap@...otime.net>
Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the
 process.

On Tue, 2010-12-28 at 17:38 +0100, Oleg Nesterov wrote: 
> This patch doesn't look right,
> 
Sorry then... :-(

> > 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.

> >  	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?
> 
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

> 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.
> 
> --- x/kernel/posix-cpu-timers.c
> +++ x/kernel/posix-cpu-timers.c
> @@ -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. 

> 	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

-- 
<<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 -- dario.faggioli@...ber.org

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)