2.6.12: itimer_real timers don't survive execve() any more

2.6.12: itimer_real timers don't survive execve() any more

[Gerd Knorr]

  Hi,

Somewhere between 2.6.11 and 2.6.12 the regression in $subject
was added to the linux kernel.  Testcase below.

Ideas on that anyone?

  Gerd

==============================[ test_exec_alarm.c ]==============================
#include <sys/time.h>
#include <unistd.h>
#include <string.h>

int main(int argc, char ** argv)
{
    struct itimerval value;
    int retval;
    static char hosted_executable[256];
    char * hosted_executable_args[2];

    /* Set the alarm timer. */
    value.it_interval.tv_sec = 0;
    value.it_interval.tv_usec = 0;
    value.it_value.tv_sec = 200;
    value.it_value.tv_usec = 0;
    retval = setitimer(ITIMER_REAL, &value, NULL);
    if (retval != 0) {
        perror("setitimer()");
        return 1;
    }

    /* Prepare the file name of the hosted executable. */
    strcpy(hosted_executable, "./test_getitimer");

    /* Prepare the command line arguments. */
    hosted_executable_args[0] = hosted_executable;
    hosted_executable_args[1] = NULL;

    /* Execute the hosted executable which should inherit the timer. */
    retval = execvp(hosted_executable, hosted_executable_args);

    /* If we get here, the execvp() call failed. */
    perror("execvp()");
    return 1;
}
==============================[ test_getitimer.c ]==============================
#include <sys/time.h>
#include <stdio.h>

int main(int argc, char ** argv)
{
    struct itimerval value;
    int retval;

    retval = getitimer(ITIMER_REAL, &value);
    if (retval != 0) {
        perror("getitimer()");
        return 1;
    }

    printf("alarm timer value: %u sec, %u msec\n", value.it_value.tv_sec, value.it_value.tv_usec);
    return 0;
}

[PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[George Anzinger]

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

Gerd Knorr wrote:
>   Hi,
> 
> Somewhere between 2.6.11 and 2.6.12 the regression in $subject
> was added to the linux kernel.  Testcase below.

Yep.  The itimer changes got a bit carried away.  Here is a fix.

-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

[-- Attachment #2: itimer-fix2.patch --]
[-- Type: text/plain, Size: 1316 bytes --]

Source: MontaVista Software, Inc. George Anzinger <george@mvista.com>
Type: Defect Fix 
Description:

 	The changes to itimer of late (after 2.6.11) cause itimers not
 	to survive the exec* calls.  Standard says they should.  

Signed-off-by: George Anzinger<george@mvista.com>

 exit.c         |    1 +
 posix-timers.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)


Index: linux-2.6.13-rc/kernel/exit.c
===================================================================
--- linux-2.6.13-rc.orig/kernel/exit.c
+++ linux-2.6.13-rc/kernel/exit.c
@@ -794,6 +794,7 @@ fastcall NORET_TYPE void do_exit(long co
 	}
 
 	tsk->flags |= PF_EXITING;
+	del_timer_sync(&tsk->signal->real_timer);
 
 	/*
 	 * Make sure we don't try to process any timer firings
Index: linux-2.6.13-rc/kernel/posix-timers.c
===================================================================
--- linux-2.6.13-rc.orig/kernel/posix-timers.c
+++ linux-2.6.13-rc/kernel/posix-timers.c
@@ -1183,10 +1183,10 @@ void exit_itimers(struct signal_struct *
 	struct k_itimer *tmr;
 
 	while (!list_empty(&sig->posix_timers)) {
-		tmr = list_entry(sig->posix_timers.next, struct k_itimer, list);
+		tmr = list_entry(sig->posix_timers.next,
+				 struct k_itimer, list);
 		itimer_delete(tmr);
 	}
-	del_timer_sync(&sig->real_timer);
 }
 
 /*

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[Roland McGrath]

That's wrong.  It has to be done only by the last thread in the group to go.
Just revert Ingo's change.


Thanks,
Roland

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[Andrew Morton]

Roland McGrath <roland@redhat.com> wrote:
>
> That's wrong.  It has to be done only by the last thread in the group to go.
> Just revert Ingo's change.
> 

OK..

--- 25/kernel/exit.c~revert-timer-exit-cleanup	Thu Aug  4 15:00:55 2005
+++ 25-akpm/kernel/exit.c	Thu Aug  4 15:01:06 2005
@@ -829,8 +829,10 @@ fastcall NORET_TYPE void do_exit(long co
 	acct_update_integrals(tsk);
 	update_mem_hiwater(tsk);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
-	if (group_dead)
+	if (group_dead) {
+ 		del_timer_sync(&tsk->signal->real_timer);
 		acct_process(code);
+	}
 	exit_mm(tsk);
 
 	exit_sem(tsk);
diff -puN kernel/posix-timers.c~revert-timer-exit-cleanup kernel/posix-timers.c
--- 25/kernel/posix-timers.c~revert-timer-exit-cleanup	Thu Aug  4 15:00:55 2005
+++ 25-akpm/kernel/posix-timers.c	Thu Aug  4 15:01:06 2005
@@ -1166,7 +1166,6 @@ void exit_itimers(struct signal_struct *
 		tmr = list_entry(sig->posix_timers.next, struct k_itimer, list);
 		itimer_delete(tmr);
 	}
-	del_timer_sync(&sig->real_timer);
 }
 
 /*
_

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[George Anzinger]

Andrew Morton wrote:
> Roland McGrath <roland@redhat.com> wrote:
> 
>>That's wrong.  It has to be done only by the last thread in the group to go.
>>Just revert Ingo's change.
>>
Hm... I was looking at 2.6.10 to figure it out.  This looks more correct.

> 
> 
> OK..
> 
> --- 25/kernel/exit.c~revert-timer-exit-cleanup	Thu Aug  4 15:00:55 2005
> +++ 25-akpm/kernel/exit.c	Thu Aug  4 15:01:06 2005
> @@ -829,8 +829,10 @@ fastcall NORET_TYPE void do_exit(long co
>  	acct_update_integrals(tsk);
>  	update_mem_hiwater(tsk);
>  	group_dead = atomic_dec_and_test(&tsk->signal->live);
> -	if (group_dead)
> +	if (group_dead) {
> + 		del_timer_sync(&tsk->signal->real_timer);
>  		acct_process(code);
> +	}
>  	exit_mm(tsk);
>  
>  	exit_sem(tsk);
> diff -puN kernel/posix-timers.c~revert-timer-exit-cleanup kernel/posix-timers.c
> --- 25/kernel/posix-timers.c~revert-timer-exit-cleanup	Thu Aug  4 15:00:55 2005
> +++ 25-akpm/kernel/posix-timers.c	Thu Aug  4 15:01:06 2005
> @@ -1166,7 +1166,6 @@ void exit_itimers(struct signal_struct *
>  		tmr = list_entry(sig->posix_timers.next, struct k_itimer, list);
>  		itimer_delete(tmr);
>  	}
> -	del_timer_sync(&sig->real_timer);
>  }
>  
>  /*
> _
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[Gerd Knorr]

On Thu, Aug 04, 2005 at 03:02:51PM -0700, Andrew Morton wrote:
> Roland McGrath <roland@redhat.com> wrote:
> >
> > That's wrong.  It has to be done only by the last thread in the group to go.
> > Just revert Ingo's change.
> > 
> 
> OK..
> 
> +++ 25-akpm/kernel/exit.c	Thu Aug  4 15:01:06 2005
> @@ -829,8 +829,10 @@ fastcall NORET_TYPE void do_exit(long co
> -	if (group_dead)
> +	if (group_dead) {
> + 		del_timer_sync(&tsk->signal->real_timer);
>  		acct_process(code);
> +	}
> +++ 25-akpm/kernel/posix-timers.c	Thu Aug  4 15:01:06 2005
> @@ -1166,7 +1166,6 @@ void exit_itimers(struct signal_struct *
> -	del_timer_sync(&sig->real_timer);

That one fixes it for me.

  Gerd

-- 
panic("it works"); /* avoid being flooded with debug messages */

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[George Anzinger]

Gerd Knorr wrote:
> On Thu, Aug 04, 2005 at 03:02:51PM -0700, Andrew Morton wrote:
> 
>>Roland McGrath <roland@redhat.com> wrote:
>>
>>>That's wrong.  It has to be done only by the last thread in the group to go.
>>>Just revert Ingo's change.
>>>
>>
>>OK..
>>
>>+++ 25-akpm/kernel/exit.c	Thu Aug  4 15:01:06 2005
>>@@ -829,8 +829,10 @@ fastcall NORET_TYPE void do_exit(long co
>>-	if (group_dead)
>>+	if (group_dead) {
>>+ 		del_timer_sync(&tsk->signal->real_timer);
>> 		acct_process(code);
>>+	}
>>+++ 25-akpm/kernel/posix-timers.c	Thu Aug  4 15:01:06 2005
>>@@ -1166,7 +1166,6 @@ void exit_itimers(struct signal_struct *
>>-	del_timer_sync(&sig->real_timer);
> 
> 
> That one fixes it for me.

There are other concerns.  Let me see if I understand this.  A thread 
(other than the leader) can exec and we then need to change the 
real_timer to wake the new task which will NOT be using the same task 
struct.

My looking at the code shows that the thread leader can exit and then 
stays around as a zombi until the last thread in the group exits.  If an 
alarm comes during this wait I suspect it will wake this zombi and cause 
problems.  So, don't we need to also change real_timer's task when the 
exiting task is the real_timer wake up task, assigning it to some other 
member of the group?  Note, I don't say just if it is the group leader...

Then when we finally release the signal structure, we can "del" the timer.

Did I miss something here?
> 
-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[Roland McGrath]

> There are other concerns.  Let me see if I understand this.  A thread 
> (other than the leader) can exec and we then need to change the 
> real_timer to wake the new task which will NOT be using the same task 
> struct.

That's correct.  de_thread will turn the thread calling exec into the new
leader and kill off all the other threads, including the old leader.  The
exec'ing thread's existing task_struct is reassigned to the PID of the
original leader.

> My looking at the code shows that the thread leader can exit and then 
> stays around as a zombi until the last thread in the group exits.  

That is correct.

> If an alarm comes during this wait I suspect it will wake this zombi and
> cause problems.

You are mistaken.  The signal code handles process signals sent when the
leader is a zombie.  The group leader sticks around with the PID that
matches the TGID, until there are no live threads with its TGID.  That is
how process-wide kill can still work.


Thanks,
Roland

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[George Anzinger]

Roland McGrath wrote:
>>There are other concerns.  Let me see if I understand this.  A thread 
>>(other than the leader) can exec and we then need to change the 
>>real_timer to wake the new task which will NOT be using the same task 
>>struct.
> 
> 
> That's correct.  de_thread will turn the thread calling exec into the new
> leader and kill off all the other threads, including the old leader.  The
> exec'ing thread's existing task_struct is reassigned to the PID of the
> original leader.
> 
> 
>>My looking at the code shows that the thread leader can exit and then 
>>stays around as a zombi until the last thread in the group exits.  
> 
> 
> That is correct.
> 
> 
>>If an alarm comes during this wait I suspect it will wake this zombi and
>>cause problems.
> 
> 
> You are mistaken.  The signal code handles process signals sent when the
> leader is a zombie.  The group leader sticks around with the PID that
> matches the TGID, until there are no live threads with its TGID.  That is
> how process-wide kill can still work.

Yes, I see, traced through the signal delivery.  So Linus' patch as well 
as the regression of Ingo's will fix all of this.  Right?

-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more

[Andrew Morton]

George Anzinger <george@mvista.com> wrote:
>
> Source: MontaVista Software, Inc. George Anzinger <george@mvista.com>
> Type: Defect Fix 
> Description:
> 
>  	The changes to itimer of late (after 2.6.11) cause itimers not
>  	to survive the exec* calls.  Standard says they should.  
> 
> Signed-off-by: George Anzinger<george@mvista.com>
> 
>  exit.c         |    1 +
>  posix-timers.c |    4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> 
> Index: linux-2.6.13-rc/kernel/exit.c
> ===================================================================
> --- linux-2.6.13-rc.orig/kernel/exit.c
> +++ linux-2.6.13-rc/kernel/exit.c
> @@ -794,6 +794,7 @@ fastcall NORET_TYPE void do_exit(long co
>  	}
>  
>  	tsk->flags |= PF_EXITING;
> +	del_timer_sync(&tsk->signal->real_timer);
>  
>  	/*
>  	 * Make sure we don't try to process any timer firings
> Index: linux-2.6.13-rc/kernel/posix-timers.c
> ===================================================================
> --- linux-2.6.13-rc.orig/kernel/posix-timers.c
> +++ linux-2.6.13-rc/kernel/posix-timers.c
> @@ -1183,10 +1183,10 @@ void exit_itimers(struct signal_struct *
>  	struct k_itimer *tmr;
>  
>  	while (!list_empty(&sig->posix_timers)) {
> -		tmr = list_entry(sig->posix_timers.next, struct k_itimer, list);
> +		tmr = list_entry(sig->posix_timers.next,
> +				 struct k_itimer, list);
>  		itimer_delete(tmr);
>  	}
> -	del_timer_sync(&sig->real_timer);
>  }

Yup, that's the one, thanks George.

Obvious though it is, I'll hold off on forwarding this Linuswards until
Gerd has had a chance to test it.  Or did you run Gerd's test app?