Re: [PATCH] kernel: release ptraced tasks before zap_pid_ns_processes

[Oleg Nesterov]

Sorry for delay, vacation,

On 01/02, Andrei Vagin wrote:
>
> zap_pid_ns_processes() can stuck on waiting tasks from the dead list. In
> this case, we will have one unkillable process with one or more dead
> children.

Thanks!

> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -664,9 +664,6 @@ static void forget_original_parent(struct task_struct 
> *father,
>  {
>       struct task_struct *p, *t, *reaper;
>  
> -     if (unlikely(!list_empty(&father->ptraced)))
> -             exit_ptrace(father, dead);
> -
>       /* Can drop and reacquire tasklist_lock */
>       reaper = find_child_reaper(father);
>       if (list_empty(&father->children))
> @@ -705,8 +702,18 @@ static void exit_notify(struct task_struct *tsk, int 
> group_dead)
>       LIST_HEAD(dead);
>  
>       write_lock_irq(&tasklist_lock);
> -     forget_original_parent(tsk, &dead);
> +     if (unlikely(!list_empty(&tsk->ptraced)))
> +             exit_ptrace(tsk, &dead);
> +     write_unlock_irq(&tasklist_lock);
> +
> +     /* Ptraced tasks have to be released before zap_pid_ns_processes(). */
> +     list_for_each_entry_safe(p, n, &dead, ptrace_entry) {
> +             list_del_init(&p->ptrace_entry);
> +             release_task(p);
> +     }
>  
> +     write_lock_irq(&tasklist_lock);
> +     forget_original_parent(tsk, &dead);
>       if (group_dead)
>               kill_orphaned_pgrp(tsk->group_leader, NULL);

How about a different fix below? It avoids additional 
write_lock/unlock(tasklist),
and another list_for_each_entry_safe(dead) loop is called only if it is actually
needed.

Or I missed something?

Oleg.


--- x/kernel/exit.c
+++ x/kernel/exit.c
@@ -558,12 +558,14 @@ static struct task_struct *find_alive_th
        return NULL;
 }
 
-static struct task_struct *find_child_reaper(struct task_struct *father)
+static struct task_struct *find_child_reaper(struct task_struct *father,
+                                               struct list_head *dead)
        __releases(&tasklist_lock)
        __acquires(&tasklist_lock)
 {
        struct pid_namespace *pid_ns = task_active_pid_ns(father);
        struct task_struct *reaper = pid_ns->child_reaper;
+       struct task_struct *p, *n;
 
        if (likely(reaper != father))
                return reaper;
@@ -579,6 +581,11 @@ static struct task_struct *find_child_re
                panic("Attempted to kill init! exitcode=0x%08x\n",
                        father->signal->group_exit_code ?: father->exit_code);
        }
+
+       list_for_each_entry_safe(p, n, &dead, ptrace_entry) {
+               list_del_init(&p->ptrace_entry);
+               release_task(p);
+       }
        zap_pid_ns_processes(pid_ns);
        write_lock_irq(&tasklist_lock);
 
@@ -668,7 +675,7 @@ static void forget_original_parent(struc
                exit_ptrace(father, dead);
 
        /* Can drop and reacquire tasklist_lock */
-       reaper = find_child_reaper(father);
+       reaper = find_child_reaper(father, dead);
        if (list_empty(&father->children))
                return;