account_group_exec_runtime: fix the racy usage of ->signal

Compile tested.

Unlike other similar routines, account_group_exec_runtime() could be called "implicitly" after exit_notify(). This means we can race with the parent doing release_task(), we can't just check ->signal != NULL.

Take ->siglock to make sure ->signal can't go away.

This is the minimal fix, with this patch we don't need need get/put cpu, and I think we should uninline this function.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 8 lines · Oleg Nesterov

kernel/sched_stats.h renamed from kernel/sched_stats.h~A_G_E_R_FIX +1 -0

@@ -351,10 +351,12 @@ static inline void account_group_exec_ru
 					      unsigned long long ns)
 {
 	struct signal_struct *sig;
+	unsigned long flags;
 

kernel/sched_stats.h static inline void account_group_exec_ru · line 357

Quoted 4 lines

kernel/sched_stats.h renamed from kernel/sched_stats.h~A_G_E_R_FIX +1 -2

-	sig = tsk->signal;
-	if (unlikely(!sig))
+	if (unlikely(!lock_task_sighand(tsk, &flags)))
 		return;

i think this will lock up:

the signal lock must not nest inside the rq
lock, and these accounting functions are called from within the scheduler.

Quoted 11 lines · Ingo Molnar

Quoted 8 lines · Oleg Nesterov

kernel/sched_stats.h renamed from kernel/sched_stats.h~A_G_E_R_FIX +1 -0

@@ -351,10 +351,12 @@ static inline void account_group_exec_ru
 					      unsigned long long ns)
 {
 	struct signal_struct *sig;
+	unsigned long flags;
 

Quoted 6 lines

kernel/sched_stats.h static inline void account_group_exec_ru · line 357

Quoted 4 lines

kernel/sched_stats.h renamed from kernel/sched_stats.h~A_G_E_R_FIX +1 -2

-	sig = tsk->signal;
-	if (unlikely(!sig))
+	if (unlikely(!lock_task_sighand(tsk, &flags)))
 		return;

i think this will lock up:

Ah. I worried about this, but convinced myself this is OK...

Quoted 3 lines

the signal lock must not nest inside the rq
lock, and these accounting functions are called from within the scheduler.

Why? we seem to never do task_rq_lock() under ->siglock ?

Quoted 2 lines · Oleg Nesterov

Quoted 1 line · Ingo Molnar

Quoted 5 lines

Quoted 3 lines

the signal lock must not nest inside the rq
lock, and these accounting functions are called from within the scheduler.

Why? we seem to never do task_rq_lock() under ->siglock ?

signal_wake_up() ?

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Oleg Nesterov

Quoted 1 line · Ingo Molnar

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

the signal lock must not nest inside the rq
lock, and these accounting functions are called from within the scheduler.

Why? we seem to never do task_rq_lock() under ->siglock ?

signal_wake_up() ?

I'd wish very much I could say I have already realized this, but I didn't. Thanks Ingo!

I don't see the good solution for this problem. I'll send the new patch in a minute, but it is ugly. Basically it is

	--- a/kernel/exit.c
	+++ b/kernel/exit.c
	@@ -141,6 +141,8 @@ static void __exit_signal(struct task_st
		if (sig) {
			flush_sigqueue(&sig->shared_pending);
			taskstats_tgid_free(sig);
	+		smp_mb();
	+		spin_unlock_wait(&task_rq(tsk)->lock);
			__cleanup_signal(sig);
		}
	 }

except this needs a helper in sched.c. You can nack it right now ;) Of course we can protect ->signal with rcu, but this is even worse imho.

Anybody sees a bettter fix?

Perhaps we can change sched.c to do update_curr() only when the task is not running (except ->task_tick), iow perhaps we can check sleep/wakeup == T before calling update_cur(). But this is not easy even if really possible.

Quoted 17 lines · Oleg Nesterov

Compile tested.

Unlike other similar routines, account_group_exec_runtime() could be called "implicitly" after exit_notify(). This means we can race with the parent doing release_task(), we can't just check ->signal != NULL.

Take ->siglock to make sure ->signal can't go away.

This is the minimal fix, with this patch we don't need need get/put cpu, and I think we should uninline this function.

Signed-off-by Oleg Nesterov <[email protected]>

kernel/sched_stats.h static inline void account_group_exec_ru · line 357

Quoted 9 lines

kernel/sched_stats.h renamed from kernel/sched_stats.h~A_G_E_R_FIX +2 -2

@@ -351,10 +351,12 @@ static inline void account_group_exec_ru
 					      unsigned long long ns)
 {
 	struct signal_struct *sig;
+	unsigned long flags;
 
-	sig = tsk->signal;
-	if (unlikely(!sig))
+	if (unlikely(!lock_task_sighand(tsk, &flags)))
 		return;

i think this will lock up: the signal lock must not nest inside the rq lock, and these accounting functions are called from within the scheduler.

Ingo

Quoted 8 lines · Ingo Molnar

Quoted 6 lines · Oleg Nesterov

(quoted diff) +1 -0

@@ -351,10 +351,12 @@ static inline void account_group_exec_ru
 					      unsigned long long ns)
 {
 	struct signal_struct *sig;
+	unsigned long flags;
 

Quoted 8 lines

Review static inline void account_group_exec_ru · line 357

Quoted 4 lines

(quoted diff) +1 -2

-	sig = tsk->signal;
-	if (unlikely(!sig))
+	if (unlikely(!lock_task_sighand(tsk, &flags)))
 		return;

i think this will lock up: the signal lock must not nest inside the rq lock, and these accounting functions are called from within the scheduler.

I can confirm that this does hang on bootup.