linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 2.6.12: itimer_real timers don't survive execve() any more
@ 2005-08-04 16:45 Gerd Knorr
  2005-08-04 21:22 ` [PATCH] " George Anzinger
  0 siblings, 1 reply; 10+ messages in thread
From: Gerd Knorr @ 2005-08-04 16:45 UTC (permalink / raw)
  To: Linux Kernel Mailing List

  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;
}

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 16:45 2.6.12: itimer_real timers don't survive execve() any more Gerd Knorr
@ 2005-08-04 21:22 ` George Anzinger
  2005-08-04 21:34   ` Roland McGrath
  2005-08-04 21:37   ` Andrew Morton
  0 siblings, 2 replies; 10+ messages in thread
From: George Anzinger @ 2005-08-04 21:22 UTC (permalink / raw)
  To: Gerd Knorr, Andrew Morton, Roland McGrath; +Cc: Linux Kernel Mailing List

[-- 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);
 }
 
 /*

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 21:22 ` [PATCH] " George Anzinger
@ 2005-08-04 21:34   ` Roland McGrath
  2005-08-04 22:02     ` Andrew Morton
  2005-08-04 21:37   ` Andrew Morton
  1 sibling, 1 reply; 10+ messages in thread
From: Roland McGrath @ 2005-08-04 21:34 UTC (permalink / raw)
  To: george; +Cc: Gerd Knorr, Andrew Morton, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 21:22 ` [PATCH] " George Anzinger
  2005-08-04 21:34   ` Roland McGrath
@ 2005-08-04 21:37   ` Andrew Morton
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2005-08-04 21:37 UTC (permalink / raw)
  To: george; +Cc: kraxel, roland, linux-kernel

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?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 21:34   ` Roland McGrath
@ 2005-08-04 22:02     ` Andrew Morton
  2005-08-04 22:10       ` George Anzinger
  2005-08-05  8:44       ` Gerd Knorr
  0 siblings, 2 replies; 10+ messages in thread
From: Andrew Morton @ 2005-08-04 22:02 UTC (permalink / raw)
  To: Roland McGrath; +Cc: george, kraxel, linux-kernel

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);
 }
 
 /*
_


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 22:02     ` Andrew Morton
@ 2005-08-04 22:10       ` George Anzinger
  2005-08-05  8:44       ` Gerd Knorr
  1 sibling, 0 replies; 10+ messages in thread
From: George Anzinger @ 2005-08-04 22:10 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Roland McGrath, kraxel, linux-kernel

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/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-04 22:02     ` Andrew Morton
  2005-08-04 22:10       ` George Anzinger
@ 2005-08-05  8:44       ` Gerd Knorr
  2005-08-05 15:33         ` George Anzinger
  1 sibling, 1 reply; 10+ messages in thread
From: Gerd Knorr @ 2005-08-05  8:44 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Roland McGrath, george, linux-kernel

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 */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-05  8:44       ` Gerd Knorr
@ 2005-08-05 15:33         ` George Anzinger
  2005-08-05 22:10           ` Roland McGrath
  0 siblings, 1 reply; 10+ messages in thread
From: George Anzinger @ 2005-08-05 15:33 UTC (permalink / raw)
  To: Gerd Knorr; +Cc: Andrew Morton, Roland McGrath, linux-kernel, Linus Torvalds

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/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-05 15:33         ` George Anzinger
@ 2005-08-05 22:10           ` Roland McGrath
  2005-08-06  0:50             ` George Anzinger
  0 siblings, 1 reply; 10+ messages in thread
From: Roland McGrath @ 2005-08-05 22:10 UTC (permalink / raw)
  To: george; +Cc: Gerd Knorr, Andrew Morton, linux-kernel, Linus Torvalds

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Re: 2.6.12: itimer_real timers don't survive execve() any more
  2005-08-05 22:10           ` Roland McGrath
@ 2005-08-06  0:50             ` George Anzinger
  0 siblings, 0 replies; 10+ messages in thread
From: George Anzinger @ 2005-08-06  0:50 UTC (permalink / raw)
  To: Roland McGrath; +Cc: Gerd Knorr, Andrew Morton, linux-kernel, Linus Torvalds

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/

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2005-08-06  0:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-08-04 16:45 2.6.12: itimer_real timers don't survive execve() any more Gerd Knorr
2005-08-04 21:22 ` [PATCH] " George Anzinger
2005-08-04 21:34   ` Roland McGrath
2005-08-04 22:02     ` Andrew Morton
2005-08-04 22:10       ` George Anzinger
2005-08-05  8:44       ` Gerd Knorr
2005-08-05 15:33         ` George Anzinger
2005-08-05 22:10           ` Roland McGrath
2005-08-06  0:50             ` George Anzinger
2005-08-04 21:37   ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).