[PATCH 1/6] fill_tgid: fix task_struct leak and possible oops

[PATCH 1/6] fill_tgid: fix task_struct leak and possible oops

[Oleg Nesterov]

1. fill_tgid() forgets to do put_task_struct(first).

2. release_task(first) can happen after fill_tgid() drops tasklist_lock,
   it is unsafe to dereference first->signal.

This is a temporary fix, imho the locking should be reworked.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- STATS/kernel/taskstats.c~1_fix_sig	2006-10-22 18:24:03.000000000 +0400
+++ STATS/kernel/taskstats.c	2006-10-26 23:44:32.000000000 +0400
@@ -237,14 +237,17 @@ static int fill_tgid(pid_t tgid, struct 
 	} else
 		get_task_struct(first);
 
-	/* Start with stats from dead tasks */
-	spin_lock_irqsave(&first->signal->stats_lock, flags);
-	if (first->signal->stats)
-		memcpy(stats, first->signal->stats, sizeof(*stats));
-	spin_unlock_irqrestore(&first->signal->stats_lock, flags);
 
 	tsk = first;
 	read_lock(&tasklist_lock);
+	/* Start with stats from dead tasks */
+	if (first->signal) {
+		spin_lock_irqsave(&first->signal->stats_lock, flags);
+		if (first->signal->stats)
+			memcpy(stats, first->signal->stats, sizeof(*stats));
+		spin_unlock_irqrestore(&first->signal->stats_lock, flags);
+	}
+
 	do {
 		if (tsk->exit_state == EXIT_ZOMBIE && thread_group_leader(tsk))
 			continue;
@@ -264,7 +267,7 @@ static int fill_tgid(pid_t tgid, struct 
 	 * Accounting subsytems can also add calls here to modify
 	 * fields of taskstats.
 	 */
-
+	put_task_struct(first);
 	return 0;
 }
 

Re: [PATCH 1/6] fill_tgid: fix task_struct leak and possible oops

[Balbir Singh]

Oleg Nesterov wrote:
> 1. fill_tgid() forgets to do put_task_struct(first).
> 

Looks good!

> 2. release_task(first) can happen after fill_tgid() drops tasklist_lock,
>    it is unsafe to dereference first->signal.
> 

But, we have a reference to first via get_task_struct(). release_task()
would do just a put_task_struct(). Am I missing something?


> This is a temporary fix, imho the locking should be reworked.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- STATS/kernel/taskstats.c~1_fix_sig	2006-10-22 18:24:03.000000000 +0400
> +++ STATS/kernel/taskstats.c	2006-10-26 23:44:32.000000000 +0400
> @@ -237,14 +237,17 @@ static int fill_tgid(pid_t tgid, struct 
>  	} else
>  		get_task_struct(first);
>  
> -	/* Start with stats from dead tasks */
> -	spin_lock_irqsave(&first->signal->stats_lock, flags);
> -	if (first->signal->stats)
> -		memcpy(stats, first->signal->stats, sizeof(*stats));
> -	spin_unlock_irqrestore(&first->signal->stats_lock, flags);
>  
>  	tsk = first;
>  	read_lock(&tasklist_lock);
> +	/* Start with stats from dead tasks */
> +	if (first->signal) {
> +		spin_lock_irqsave(&first->signal->stats_lock, flags);
> +		if (first->signal->stats)
> +			memcpy(stats, first->signal->stats, sizeof(*stats));
> +		spin_unlock_irqrestore(&first->signal->stats_lock, flags);
> +	}
> +
>  	do {
>  		if (tsk->exit_state == EXIT_ZOMBIE && thread_group_leader(tsk))
>  			continue;
> @@ -264,7 +267,7 @@ static int fill_tgid(pid_t tgid, struct 
>  	 * Accounting subsytems can also add calls here to modify
>  	 * fields of taskstats.
>  	 */
> -
> +	put_task_struct(first);
>  	return 0;
>  }
>  
> 


-- 

	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Re: [PATCH 1/6] fill_tgid: fix task_struct leak and possible oops

[Oleg Nesterov]

On 10/30, Balbir Singh wrote:
>
> Oleg Nesterov wrote:
> 
> > 2. release_task(first) can happen after fill_tgid() drops tasklist_lock,
> >    it is unsafe to dereference first->signal.
> > 
> 
> But, we have a reference to first via get_task_struct(). release_task()
> would do just a put_task_struct(). Am I missing something?

No, release_task() will reap the task. tsk->usage protects only task_struct
itself (more precisely, it protects against __put_task_struct()). And please
note that release_task()->__exit_signal() sets tsk->signal = NULL.


QUESTION: taskstats_exit_alloc() does kfree(kmem_cache_alloc()), is it OK?
Yes, it works, but is it good? The comment says:

	* @objp: pointer returned by kmalloc.


Another question,

	do_exit()
		taskstats_exit_alloc()
		...
		taskstats_exit_send()
		taskstats_exit_free()

What is the point? Why can't we have taskstats_exit() which does alloc+send+free
itself? This looks like unnecessary complication to me.

>From taskstats_exit_alloc:

	/*
	 * This is the cpu on which the task is exiting currently and will
	 * be the one for which the exit event is sent, even if the cpu
	 * on which this function is running changes later.
	 */

Why do we record current cpu exactly here? This task probably changed its
CPU many times since it entered sys_exit(), so what is the problem if it
will change CPU again before taskstats_exit_send() ?

Oleg.