[Devel] [PATCH RHEL7 COMMIT] ms/exit: reparent: fix the cross-namespace PR_SET_CHILD_SUBREAPER reparenting

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-1160.62.1.vz7.187.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-1160.62.1.vz7.187.5
------>
commit 33f72c55b12c4db52f9d16093c4a2f3a81825f5b
Author: Oleg Nesterov <oleg at redhat.com>
Date:   Wed Jun 15 17:54:18 2022 +0300

    ms/exit: reparent: fix the cross-namespace PR_SET_CHILD_SUBREAPER reparenting
    
    find_new_reaper() assumes that "has_child_subreaper" logic is safe as
    long as we are not the exiting ->child_reaper and this is doubly wrong:
    
    1. In fact it is safe if "pid_ns->child_reaper == father"; there must
       be no children after zap_pid_ns_processes() returns, so it doesn't
       matter what we return in this case and even pid_ns->child_reaper is
       wrong otherwise: we can't reparent to ->child_reaper == current.
    
       This is not a bug, but this is confusing.
    
    2. It is not safe if we are not pid_ns->child_reaper but from the same
       thread group. We drop tasklist_lock before zap_pid_ns_processes(),
       so another thread can lock it and choose the new reaper from the
       upper namespace if has_child_subreaper == T, and this is obviously
       wrong.
    
       This is not that bad, zap_pid_ns_processes() won't return until the
       the new reaper reaps all zombies, but this should be fixed anyway.
    
    We could change for_each_thread() loop to use ->exit_state instead of
    PF_EXITING which we had to use until 8aac62706ada, or we could change
    copy_signal() to check CLONE_NEWPID before setting has_child_subreaper,
    but lets change this code so that it is clear we can't look outside of
    our namespace, otherwise same_thread_group(reaper, child_reaper) check
    will look wrong and confusing anyway.
    
    We can simply start from "father" and fix the problem. We can't wrongly
    return a thread from the same thread group if ->is_child_subreaper == T,
    we know that all threads have PF_EXITING set.
    
    Signed-off-by: Oleg Nesterov <oleg at redhat.com>
    Cc: Aaron Tomlin <atomlin at redhat.com>
    Cc: "Eric W. Biederman" <ebiederm at xmission.com>
    Cc: Kay Sievers <kay at vrfy.org>
    Cc: Lennart Poettering <lennart at poettering.net>
    Cc: Sterling Alexander <stalexan at redhat.com>
    Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
    
    (cherry picked from ms commit 7d24e2df52f596a1ea922e4f84a61f2fb24fbb70)
    Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
    
    =================
    Patchset description:
    vz7: fix child-reaper reparenting
    
    Forth patch is needed as kernel can reparent process to a dead thread
    which is wrong.
    
    Third patch is needed as kernel could reparent process from father from
    one pidns to process from different pidns, which creates configurations
    not supported by CRIU. Found it when reproducing problem from CRIU
    mainstream issue in VZ7 ct.
    
    https://github.com/checkpoint-restore/criu/issues/1914
    
    First and Second are just to make it apply cleaner.
    
    Oleg Nesterov (4):
      exit: reparent: fix the cross-namespace PR_SET_CHILD_SUBREAPER
        reparenting
      exit: reparent: document the ->has_child_subreaper checks
      exit: fix the setns() && PR_SET_CHILD_SUBREAPER interaction
      exit: reparent: fix the dead-parent PR_SET_CHILD_SUBREAPER reparenting
---
 kernel/exit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 65db4d4a29fe..ff6c58f12d8f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -639,7 +639,9 @@ static struct task_struct *find_new_reaper(struct task_struct *father)
 
 		zap_pid_ns_processes(pid_ns);
 		tasklist_write_lock_irq();
-	} else if (father->signal->has_child_subreaper) {
+	}
+
+	if (father->signal->has_child_subreaper) {
 		struct task_struct *reaper;
 
 		/*
@@ -649,7 +651,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father)
 		 * PID namespace. However we still need the check above, see
 		 * http://marc.info/?l=linux-kernel&m=131385460420380
 		 */
-		for (reaper = father->real_parent;
+		for (reaper = father;
 		     reaper != &init_task;
 		     reaper = reaper->real_parent) {
 			if (same_thread_group(reaper, pid_ns->child_reaper))