[PATCH] posix_cpu_timers: fix timer never expires when executes clock_nanosleep

[PATCH] posix_cpu_timers: fix timer never expires when executes clock_nanosleep

[Dong Zhu]

>From c7439b90b0794c016b29356f0e232f7413ef7b60 Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Thu, 1 Aug 2013 11:39:04 +0800

When use the current process pid as the clockid, then executes
clock_nanosleep syscall the timer will never expire. Kernel should
prevent user doing like this and this patch is supposed to fix it.I
wrote a simple case to test it:

    #include <time.h>
    #include <errno.h>
    #include <unistd.h>
    #include <sys/types.h>

    #define CPU_CLOCK_PROF           0
    #define CPU_CLOCK_VIRT           1
    #define CPU_CLOCK_SCHED          2

    #define CPU_CLOCK_THREAD         4
    #define PID_TO_CLOCKID(pid, clock) ((~(clockid_t) (pid) << 3) | (clockid_t) (clock))

    int main(void)
    {
            int ret;
            pid_t pid;
            clockid_t clk;
            struct timespec ts;

            ts.tv_sec = 1;
            ts.tv_nsec = 0;

            pid = getpid();
            clk = PID_TO_CLOCKID(pid, CPU_CLOCK_PROF);
            if ((ret = clock_nanosleep(clk, 0, &ts, NULL)) != 0) {
                    perror("clock_nanosleep");
                    return ret;
            }

            return 0;
    }

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
---
 kernel/posix-cpu-timers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index c7f31aa..cc03290 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -1413,9 +1413,9 @@ static int posix_cpu_nsleep(const clockid_t which_clock, int flags,
 	/*
 	 * Diagnose required errors first.
 	 */
-	if (CPUCLOCK_PERTHREAD(which_clock) &&
-	    (CPUCLOCK_PID(which_clock) == 0 ||
-	     CPUCLOCK_PID(which_clock) == current->pid))
+	if (CPUCLOCK_PID(which_clock) == current->pid ||
+	    (CPUCLOCK_PERTHREAD(which_clock) &&
+	     CPUCLOCK_PID(which_clock) == 0))
 		return -EINVAL;
 
 	error = do_cpu_nanosleep(which_clock, flags, rqtp, &it);
-- 
1.7.11.7


-- 
Best Regards,
Dong Zhu

Re: [PATCH] posix_cpu_timers: fix timer never expires when executes clock_nanosleep

[Stanislaw Gruszka]

Hi Dong Zhu

On Thu, Aug 01, 2013 at 06:10:19PM +0800, Dong Zhu wrote:
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index c7f31aa..cc03290 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -1413,9 +1413,9 @@ static int posix_cpu_nsleep(const clockid_t which_clock, int flags,
>  	/*
>  	 * Diagnose required errors first.
>  	 */
> -	if (CPUCLOCK_PERTHREAD(which_clock) &&
> -	    (CPUCLOCK_PID(which_clock) == 0 ||
> -	     CPUCLOCK_PID(which_clock) == current->pid))
> +	if (CPUCLOCK_PID(which_clock) == current->pid ||
> +	    (CPUCLOCK_PERTHREAD(which_clock) &&
> +	     CPUCLOCK_PID(which_clock) == 0))
>  		return -EINVAL;

Nope, this is wrong. We have to allow own pid process clock, because it
can be used correctly on multi-threaded processes. Own tid thread clock
has no sense and we correctly return -EINVAL in such case.

We could possibly add check for own pid together with check if process 
consist of one thread, but that is too complicated IMHO especially
taking into account that threads on the process can be destroyed and
created dynamically.

Stanislaw

Re: [PATCH] posix_cpu_timers: fix timer never expires when executes clock_nanosleep

[Dong Zhu]

Hi Stanislaw,

Thansk for your info.

On Thu, Aug 01, 2013 at 01:30:50PM +0200, Stanislaw Gruszka wrote:
> Hi Dong Zhu
> 
> On Thu, Aug 01, 2013 at 06:10:19PM +0800, Dong Zhu wrote:
> > diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> > index c7f31aa..cc03290 100644
> > --- a/kernel/posix-cpu-timers.c
> > +++ b/kernel/posix-cpu-timers.c
> > @@ -1413,9 +1413,9 @@ static int posix_cpu_nsleep(const clockid_t which_clock, int flags,
> >  	/*
> >  	 * Diagnose required errors first.
> >  	 */
> > -	if (CPUCLOCK_PERTHREAD(which_clock) &&
> > -	    (CPUCLOCK_PID(which_clock) == 0 ||
> > -	     CPUCLOCK_PID(which_clock) == current->pid))
> > +	if (CPUCLOCK_PID(which_clock) == current->pid ||
> > +	    (CPUCLOCK_PERTHREAD(which_clock) &&
> > +	     CPUCLOCK_PID(which_clock) == 0))
> >  		return -EINVAL;
> 
> Nope, this is wrong. We have to allow own pid process clock, because it
> can be used correctly on multi-threaded processes. Own tid thread clock

Yes, you are right, I really neglected this point.

> has no sense and we correctly return -EINVAL in such case.

> 
> We could possibly add check for own pid together with check if process 
> consist of one thread, but that is too complicated IMHO especially
> taking into account that threads on the process can be destroyed and
> created dynamically.
> 

Agree, really so complicated.

-- 
Best Regards,
Dong Zhu