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 <[email protected]>
--- 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;
}
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 <[email protected]>
>
> --- 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
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.