Re: 2.6.12: itimer_real timers don't survive execve() any more
diff mbox series

Message ID 42F28707.7060806@mvista.com
State New, archived
Headers show
Series
  • Re: 2.6.12: itimer_real timers don't survive execve() any more
Related show

Commit Message

George Anzinger Aug. 4, 2005, 9:22 p.m. UTC
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.

Comments

Andrew Morton Aug. 4, 2005, 9:37 p.m. UTC | #1
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?

-
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/
Andrew Morton Aug. 4, 2005, 10:02 p.m. UTC | #2
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);
 }
 
 /*
George Anzinger Aug. 4, 2005, 10:10 p.m. UTC | #3
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/
>
Gerd Hoffmann Aug. 5, 2005, 8:44 a.m. UTC | #4
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
George Anzinger Aug. 5, 2005, 3:33 p.m. UTC | #5
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?
>

Patch
diff mbox series

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