clock_nanosleep() task_struct leak

Hello,

Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:

-----8<-----8<-----8<-----
#include <time.h>

static const struct timespec req;

int main(void) {
        return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
                        TIMER_ABSTIME, &req, NULL);
}

-----8<-----8<-----8<-----

Quoted 7 lines · Tommi Rantala

Hello,

Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:

-----8<-----8<-----8<-----
#include <time.h>

Quoted 7 lines

static const struct timespec req;

int main(void) {
        return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
                        TIMER_ABSTIME, &req, NULL);
}

-----8<-----8<-----8<-----

Groan. That code probably has more than that leak burried inside.

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Tommi Rantala

Hello,

Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:

-----8<-----8<-----8<-----
#include <time.h>

Quoted 7 lines

Quoted 7 lines

static const struct timespec req;

int main(void) {
        return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
                        TIMER_ABSTIME, &req, NULL);
}

-----8<-----8<-----8<-----

posix_cpu_timer_create()->get_task_struct() I guess...

Cough. I am not sure I ever understood this code, but now it certainly looks as if I never saw it before.

Quoted 21 lines · Oleg Nesterov

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Tommi Rantala

Hello,

Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:

-----8<-----8<-----8<-----
#include <time.h>

Quoted 7 lines

Quoted 7 lines

static const struct timespec req;

int main(void) {
        return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
                        TIMER_ABSTIME, &req, NULL);
}

-----8<-----8<-----8<-----

posix_cpu_timer_create()->get_task_struct() I guess...

Quoted 2 lines

Cough. I am not sure I ever understood this code, but now it certainly looks as if I never saw it before.

Looks on do_cpu_nanosleep() we call posix_cpu_timer_create(), but we do not call posix_cpu_timer_del() at the end. Fix will not be super simple, since we need to care about error cases. I can cook a patch if nobody else want to do this.

Quoted 25 lines · Stanislaw Gruszka

Quoted 23 lines · Oleg Nesterov

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Tommi Rantala

Hello,

Trinity discovered a task_struct leak with clock_nanosleep(), reproducible with:

-----8<-----8<-----8<-----
#include <time.h>

Quoted 7 lines

Quoted 7 lines

static const struct timespec req;

int main(void) {
        return clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID,
                        TIMER_ABSTIME, &req, NULL);
}

-----8<-----8<-----8<-----

posix_cpu_timer_create()->get_task_struct() I guess...

Cough. I am not sure I ever understood this code, but now it certainly looks as if I never saw it before.

Quoted 4 lines

Looks on do_cpu_nanosleep() we call posix_cpu_timer_create(), but we do not call posix_cpu_timer_del() at the end. Fix will not be super simple, since we need to care about error cases. I can cook a patch if nobody else want to do this.

Would be much appreciated!

Below is proposed fix. Error cases wasn't that bad since there are various limitations when timer could be fired (i.e. timer which already fired can not be fired again).

Tommi, please check if patch really fixes the problem. I tested it with signal interrupt and timeout scenarios, but I don't know how to confirm if it fix the leak or not.

kernel/posix-cpu-timers.c +10 -1

index 125cb67..07a38b6 100644
@@ -1424,6 +1424,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
                                /*
                                 * Our timer fired and was reset.
                                 */
+                               posix_cpu_timer_del(&timer);
                                spin_unlock_irq(&timer.it_lock);
                                return 0;
                        }

@@ -1441,9 +1442,17 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
                 * We were interrupted by a signal.
                 */
                sample_to_timespec(which_clock, timer.it.cpu.expires, rqtp);
-               posix_cpu_timer_set(&timer, 0, &zero_it, it);
+               error = posix_cpu_timer_set(&timer, 0, &zero_it, it);
+               if (!error)
+                       posix_cpu_timer_del(&timer);
                spin_unlock_irq(&timer.it_lock);
 
+               while (error == TIMER_RETRY) {
+                       spin_lock_irq(&timer.it_lock);
+                       error = posix_cpu_timer_del(&timer);
+                       spin_unlock_irq(&timer.it_lock);
+               }
+
                if ((it->it_value.tv_sec | it->it_value.tv_nsec) == 0) {
                        /*
                         * It actually did fire already.

In do_cpu_nanosleep() we do posix_cpu_timer_create(), but forgot corresponding posix_cpu_timer_del(), what lead to task_struct leak.

kernel/posix-cpu-timers.c +1 -0

@@ -1403,6 +1403,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 				/*
 				 * Our timer fired and was reset.
 				 */
+				posix_cpu_timer_del(&timer);
 				spin_unlock_irq(&timer.it_lock);
 				return 0;
 			}

@@ -1420,9 +1421,17 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 		 * We were interrupted by a signal.
 		 */

First of all, thank you so much. I knew it was a good idea to cc you ;)

Quoted 3 lines

In do_cpu_nanosleep() we do posix_cpu_timer_create(), but forgot corresponding posix_cpu_timer_del(), what lead to task_struct leak.

Plus, it seems we can leave the timer on ->cpu_timers list...

Quoted 18 lines

kernel/posix-cpu-timers.c +4 -1

@@ -1403,6 +1403,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 				/*
 				 * Our timer fired and was reset.
 				 */
+				posix_cpu_timer_del(&timer);
 				spin_unlock_irq(&timer.it_lock);
 				return 0;
 			}

@@ -1420,9 +1421,17 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 		 * We were interrupted by a signal.
 		 */
 		sample_to_timespec(which_clock, timer.it.cpu.expires, rqtp);
-		posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		error = posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		if (!error)
+			posix_cpu_timer_del(&timer);
 		spin_unlock_irq(&timer.it_lock);
 

kernel/posix-cpu-timers.c static int do_cpu_nanosleep(const clockid_t which_clock, int flags, · line 1431

Quoted 3 lines

kernel/posix-cpu-timers.c +3 -0

+		while (error == TIMER_RETRY) {
+			spin_lock_irq(&timer.it_lock);
+			error = posix_cpu_timer_del(&timer);

It is not clear to me why other posix_cpu_timer_del's above can't fail.. May be you can add a comment.

And I am not sure that TIMER_RETRY is the only error we should worry. And perhaps we need even more posix_cpu_timer_del's?

For example. Suppose that posix_cpu_timer_create() succeeds and does get_task_struct(p). But than p dies, and the first posix_cpu_timer_set() fails with -ESRCH. No?

Quoted 3 lines · Oleg Nesterov

It is not clear to me why other posix_cpu_timer_del's above can't fail.. May be you can add a comment.

Sure, I'll add more comments.

Once posix_cpu_timer_set(..., &zero_it, it) succeed with 0 return value, it's not possible to fire timer, so posix_cpu_timer_del() will not fail. Similar assumption is with first posix_cpu_timer_del() call I added in the patch.

Quoted 6 lines

And I am not sure that TIMER_RETRY is the only error we should worry. And perhaps we need even more posix_cpu_timer_del's?

For example. Suppose that posix_cpu_timer_create() succeeds and does get_task_struct(p). But than p dies, and the first posix_cpu_timer_set() fails with -ESRCH. No?

On second -ESRCH case posix_cpu_timer_set() internally call put_task_struct().

It does not remove from cpu_timers list, but
that is done at exit().

BTW: I don't think we handle correctly case when traced process -
- timer->it.cpu.task will die. Tracing process - timer->it_process will probably not be woken up.

In do_cpu_nanosleep() we do posic_cpu_timer_create(), but forgot corresponding posix_cpu_timer_del() what lead to task_struct leak.

Reported-and-tested-by Tommi Rantala <[email protected]>

Signed-off-by Stanislaw Gruszka <[email protected]>

---
v1 -> v2: add comments

 kernel/posix-cpu-timers.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

kernel/posix-cpu-timers.c +21 -2

index a278cad..942ca27 100644
@@ -1401,8 +1401,10 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 		while (!signal_pending(current)) {
 			if (timer.it.cpu.expires.sched == 0) {
 				/*
-				 * Our timer fired and was reset.
+				 * Our timer fired and was reset, below
+				 * deletion can not fail.
 				 */
+				posix_cpu_timer_del(&timer);
 				spin_unlock_irq(&timer.it_lock);
 				return 0;
 			}

@@ -1420,9 +1422,26 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 		 * We were interrupted by a signal.
 		 */
 		sample_to_timespec(which_clock, timer.it.cpu.expires, rqtp);
-		posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		error = posix_cpu_timer_set(&timer, 0, &zero_it, it);
+		if (!error) {
+			/*
+			 * Timer is now unarmed, deletion can not fail.
+			 */
+			posix_cpu_timer_del(&timer);
+		}
 		spin_unlock_irq(&timer.it_lock);
 
+		while (error == TIMER_RETRY) {
+			/*
+			 * We need to handle case when timer was or is in the
+			 * middle of firing. In other cases we already freed
+			 * resources.
+			 */
+			spin_lock_irq(&timer.it_lock);
+			error = posix_cpu_timer_del(&timer);
+			spin_unlock_irq(&timer.it_lock);
+		}
+
 		if ((it->it_value.tv_sec | it->it_value.tv_nsec) == 0) {
 			/*
 			 * It actually did fire already.

Signature · 1 line

1.7.11.7