Re: [PATCH] exit: call add_device_randomness() without tasklist_lock

[Oleg Nesterov <oleg@redhat.com>]

On 01/28, Mateusz Guzik wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -141,13 +141,14 @@ static void __unhash_process(struct task_struct *p, bool group_dead)
>  /*
>   * This function expects the tasklist_lock write-locked.
>   */
> -static void __exit_signal(struct task_struct *tsk)
> +static unsigned long long __exit_signal(struct task_struct *tsk)
>  {
>  	struct signal_struct *sig = tsk->signal;
>  	bool group_dead = thread_group_leader(tsk);
>  	struct sighand_struct *sighand;
>  	struct tty_struct *tty;
>  	u64 utime, stime;
> +	unsigned long long randomness;
>  
>  	sighand = rcu_dereference_check(tsk->sighand,
>  					lockdep_tasklist_lock_is_held());
> @@ -174,8 +175,7 @@ static void __exit_signal(struct task_struct *tsk)
>  			sig->curr_target = next_thread(tsk);
>  	}
>  
> -	add_device_randomness((const void*) &tsk->se.sum_exec_runtime,
> -			      sizeof(unsigned long long));
> +	randomness = tsk->se.sum_exec_runtime;
>  
>  	/*
>  	 * Accumulate here the counters for all threads as they die. We could
> @@ -214,6 +214,8 @@ static void __exit_signal(struct task_struct *tsk)
>  		flush_sigqueue(&sig->shared_pending);
>  		tty_kref_put(tty);
>  	}
> +
> +	return randomness;
>  }
>  
>  static void delayed_put_task_struct(struct rcu_head *rhp)
> @@ -242,6 +244,7 @@ void release_task(struct task_struct *p)
>  	struct task_struct *leader;
>  	struct pid *thread_pid;
>  	int zap_leader;
> +	unsigned long long randomness;
>  repeat:
>  	/* don't need to get the RCU readlock here - the process is dead and
>  	 * can't be modifying its own credentials. But shut RCU-lockdep up */
> @@ -254,7 +257,7 @@ void release_task(struct task_struct *p)
>  	write_lock_irq(&tasklist_lock);
>  	ptrace_release_task(p);
>  	thread_pid = get_pid(p->thread_pid);
> -	__exit_signal(p);
> +	randomness = __exit_signal(p);
>  
>  	/*
>  	 * If we are the last non-leader member of the thread
> @@ -280,6 +283,8 @@ void release_task(struct task_struct *p)
>  	put_pid(thread_pid);
>  	release_thread(p);
>  	put_task_struct_rcu_user(p);
> +	add_device_randomness((const void*) &randomness,
> +			      sizeof(unsigned long long));
>  
>  	p = leader;
>  	if (unlikely(zap_leader))

Hmm, why not simply shift add_device_randomness(sum_exec_runtime) to
release_release_task() ?

Oleg.

--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -174,9 +174,6 @@ static void __exit_signal(struct task_struct *tsk)
 			sig->curr_target = next_thread(tsk);
 	}
 
-	add_device_randomness((const void*) &tsk->se.sum_exec_runtime,
-			      sizeof(unsigned long long));
-
 	/*
 	 * Accumulate here the counters for all threads as they die. We could
 	 * skip the group leader because it is the last user of signal_struct,
@@ -279,6 +276,8 @@ void release_task(struct task_struct *p)
 	proc_flush_pid(thread_pid);
 	put_pid(thread_pid);
 	release_thread(p);
+	add_device_randomness((const void*)&p->se.sum_exec_runtime,
+			      sizeof(unsigned long long));
 	put_task_struct_rcu_user(p);
 
 	p = leader;