[PATCH V2] exit: trigger panic when global init has exited

[PATCH V2] exit: trigger panic when global init has exited

[Qianli Zhao]

From: Qianli Zhao <zhaoqianli@xiaomi.com>

When init sub-threads running on different CPUs exit at the same time,
zap_pid_ns_processe()->BUG() may be happened.
And every thread status is abnormal after exit(PF_EXITING set,task->mm=NULL etc),
which makes it difficult to parse coredump from fulldump normally.
In order to fix the above problem, when any one init has been set to SIGNAL_GROUP_EXIT,
trigger panic immediately, and prevent other init threads from continuing to exit.

[   24.705376] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
[   24.705382] CPU: 4 PID: 552 Comm: init Tainted: G S         O    4.14.180-perf-g4483caa8ae80-dirty #1
[   24.705390] kernel BUG at include/linux/pid_namespace.h:98!

PID: 552   CPU: 4   COMMAND: "init"
PID: 1     CPU: 7   COMMAND: "init"
core4                           core7
...                             sys_exit_group()
                                do_group_exit()
                                   - sig->flags = SIGNAL_GROUP_EXIT
                                   - zap_other_threads()
                                do_exit() //PF_EXITING is set
ret_to_user()
do_notify_resume()
get_signal()
    - signal_group_exit
    - goto fatal;
do_group_exit()
do_exit() //PF_EXITING is set
    - panic("Attempted to kill init! exitcode=0x%08x\n")
                                exit_notify()
                                find_alive_thread() //no alive sub-threads
                                zap_pid_ns_processes()//CONFIG_PID_NS is not set
                                BUG()

Signed-off-by: Qianli Zhao <zhaoqianli@xiaomi.com>
---
V2:
- Changelog update
- Remove wrong useage of SIGNAL_UNKILLABLE. 
- Add thread_group_empty() test to handle single init thread exit

---
 kernel/exit.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e3..9d2716b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -767,6 +767,17 @@ void __noreturn do_exit(long code)
 	validate_creds_for_do_exit(tsk);
 
 	/*
+	 * If global init has exited,
+	 * panic immediately to get a useable coredump.
+	 */
+	if (unlikely(is_global_init(tsk) &&
+	    (thread_group_empty(tsk) ||
+	    (tsk->signal->flags & SIGNAL_GROUP_EXIT)))) {
+			panic("Attempted to kill init! exitcode=0x%08x\n",
+				tsk->signal->group_exit_code ?: (int)code);
+	}
+
+	/*
 	 * We're taking recursive faults here in do_exit. Safest is to just
 	 * leave this task alone and wait for reboot.
 	 */
@@ -786,14 +797,6 @@ void __noreturn do_exit(long code)
 	acct_update_integrals(tsk);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
-		/*
-		 * If the last thread of global init has exited, panic
-		 * immediately to get a useable coredump.
-		 */
-		if (unlikely(is_global_init(tsk)))
-			panic("Attempted to kill init! exitcode=0x%08x\n",
-				tsk->signal->group_exit_code ?: (int)code);
-
 #ifdef CONFIG_POSIX_TIMERS
 		hrtimer_cancel(&tsk->signal->real_timer);
 		exit_itimers(tsk->signal);
-- 
1.9.1

Re: [PATCH V2] exit: trigger panic when global init has exited

[Oleg Nesterov]

On 03/12, Qianli Zhao wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -767,6 +767,17 @@ void __noreturn do_exit(long code)
>  	validate_creds_for_do_exit(tsk);
>  
>  	/*
> +	 * If global init has exited,
> +	 * panic immediately to get a useable coredump.
> +	 */
> +	if (unlikely(is_global_init(tsk) &&
> +	    (thread_group_empty(tsk) ||
> +	    (tsk->signal->flags & SIGNAL_GROUP_EXIT)))) {
> +			panic("Attempted to kill init! exitcode=0x%08x\n",
> +				tsk->signal->group_exit_code ?: (int)code);

See our discussion with Eric, this is not right.
https://lore.kernel.org/lkml/20210310173236.GB8973@redhat.com/

Oleg.

Re: [PATCH V2] exit: trigger panic when global init has exited

[Eric W. Biederman]

Qianli Zhao <zhaoqianligood@gmail.com> writes:

> From: Qianli Zhao <zhaoqianli@xiaomi.com>
>
> When init sub-threads running on different CPUs exit at the same time,
> zap_pid_ns_processe()->BUG() may be happened.
> And every thread status is abnormal after exit(PF_EXITING set,task->mm=NULL etc),
> which makes it difficult to parse coredump from fulldump normally.
> In order to fix the above problem, when any one init has been set to SIGNAL_GROUP_EXIT,
> trigger panic immediately, and prevent other init threads from continuing to exit.
>
> [   24.705376] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
> [   24.705382] CPU: 4 PID: 552 Comm: init Tainted: G S         O    4.14.180-perf-g4483caa8ae80-dirty #1
> [   24.705390] kernel BUG at include/linux/pid_namespace.h:98!
>
> PID: 552   CPU: 4   COMMAND: "init"
> PID: 1     CPU: 7   COMMAND: "init"
> core4                           core7
> ...                             sys_exit_group()
>                                 do_group_exit()
>                                    - sig->flags = SIGNAL_GROUP_EXIT
>                                    - zap_other_threads()
>                                 do_exit() //PF_EXITING is set
> ret_to_user()
> do_notify_resume()
> get_signal()
>     - signal_group_exit
>     - goto fatal;
> do_group_exit()
> do_exit() //PF_EXITING is set
>     - panic("Attempted to kill init! exitcode=0x%08x\n")
>                                 exit_notify()
>                                 find_alive_thread() //no alive sub-threads
>                                 zap_pid_ns_processes()//CONFIG_PID_NS is not set
>                                 BUG()
>
> Signed-off-by: Qianli Zhao <zhaoqianli@xiaomi.com>

The changelog is much better thank you.

As Oleg pointer out we need to do something like the code below.

diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e35e69a..bc676c06ef9a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -785,15 +785,16 @@ void __noreturn do_exit(long code)
 		sync_mm_rss(tsk->mm);
 	acct_update_integrals(tsk);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
+	/*
+	 * If the global init has exited, panic immediately to get a
+	 * useable coredump.
+	 */
+	if (unlikely(is_global_init(tsk) &&
+		     (group_dead || (tsk->signal->flags & SIGNAL_GROUP_EXIT)))) {
+		panic("Attempted to kill init! exitcode=0x%08x\n",
+		      tsk->signal->group_exit_code ?: (int)code);
+	}
 	if (group_dead) {
-		/*
-		 * If the last thread of global init has exited, panic
-		 * immediately to get a useable coredump.
-		 */
-		if (unlikely(is_global_init(tsk)))
-			panic("Attempted to kill init! exitcode=0x%08x\n",
-				tsk->signal->group_exit_code ?: (int)code);
-
 #ifdef CONFIG_POSIX_TIMERS
 		hrtimer_cancel(&tsk->signal->real_timer);
 		exit_itimers(tsk->signal);

There is still a race that could lead to the BUG in zap_pid_ns_processes.
We still have a case where the last two threads of a process call
pthread_exit (aka do_exit not do_group_exit in the kernel).

Thread A                            Thread B
do_exit()                           do_exit()

 exit_signals()
   tsk->flags |= PF_EXITING;
 group_dead = false;
                                    exit_signals()
                                      tsk->flags |= PF_EXITING;
 exit_notify()
  forget_original_parent
    find_child_reaper
      reaper = find_alive_thread()
      zap_pid_ns_processes()
         BUG()
                                    group_dead = true;
                                    if (is_global_init())
                                    	panic("Attemted to kill init");

As we are guaranteed to see the panic with my change above I suggest
we augment it by simply removing the BUG in zap_pid_ns_processes.

Or maybe not if there is a better way to write the panic code.  I don't
think having pid namespaces compiled out is a particularly common case.
So whatever we can do to keep the code correct and reduce testing.

Eric

Re: [PATCH V2] exit: trigger panic when global init has exited

[qianli zhao]

Hi Eric,Oleg

> As Oleg pointer out we need to do something like the code below.
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 04029e35e69a..bc676c06ef9a 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -785,15 +785,16 @@ void __noreturn do_exit(long code)
>                 sync_mm_rss(tsk->mm);
>         acct_update_integrals(tsk);
>         group_dead = atomic_dec_and_test(&tsk->signal->live);
> +       /*
> +        * If the global init has exited, panic immediately to get a
> +        * useable coredump.
> +        */
> +       if (unlikely(is_global_init(tsk) &&
> +                    (group_dead || (tsk->signal->flags & SIGNAL_GROUP_EXIT)))) {
> +               panic("Attempted to kill init! exitcode=0x%08x\n",
> +                     tsk->signal->group_exit_code ?: (int)code);
> +       }
>         if (group_dead) {
> -               /*
> -                * If the last thread of global init has exited, panic
> -                * immediately to get a useable coredump.
> -                */
> -               if (unlikely(is_global_init(tsk)))
> -                       panic("Attempted to kill init! exitcode=0x%08x\n",
> -                               tsk->signal->group_exit_code ?: (int)code);
> -
>  #ifdef CONFIG_POSIX_TIMERS
>                 hrtimer_cancel(&tsk->signal->real_timer);
>                 exit_itimers(tsk->signal);
>

Sorry,i missed this discussion,we needs use group_dead to replace
thread_group_empty().

> There is still a race that could lead to the BUG in zap_pid_ns_processes.
> We still have a case where the last two threads of a process call
> pthread_exit (aka do_exit not do_group_exit in the kernel).
>
> Thread A                            Thread B
> do_exit()                           do_exit()
>
>  exit_signals()
>    tsk->flags |= PF_EXITING;
>  group_dead = false;
>                                     exit_signals()
>                                       tsk->flags |= PF_EXITING;
>  exit_notify()
>   forget_original_parent
>     find_child_reaper
>       reaper = find_alive_thread()
>       zap_pid_ns_processes()
>          BUG()
>                                     group_dead = true;
>                                     if (is_global_init())
>                                         panic("Attemted to kill init");
>

My patch moves panic to before setting PF_EXITING,it can avoid this case.
What if we move atomic_dec_and_test() to before exit_signals()?
Such as:
        validate_creds_for_do_exit(tsk);

+      group_dead = atomic_dec_and_test(&tsk->signal->live);
+       /*
+        * If global init has exited,
+        * panic immediately to get a useable coredump.
+        */
+       if (unlikely(is_global_init(tsk) &&
+           (group_dead || (tsk->signal->flags & SIGNAL_GROUP_EXIT)))) {
+                       panic("Attempted to kill init! exitcode=0x%08x\n",
+                               tsk->signal->group_exit_code ?: (int)code);
+       }
...
       exit_signals(tsk);  /* sets PF_EXITING */
...
        acct_update_integrals(tsk);
-       group_dead = atomic_dec_and_test(&tsk->signal->live);
        if (group_dead) {
-               /*
-                * If the last thread of global init has exited, panic
-                * immediately to get a useable coredump.
-                */
-               if (unlikely(is_global_init(tsk)))
-                       panic("Attempted to kill init! exitcode=0x%08x\n",
-                               tsk->signal->group_exit_code ?: (int)code);

Thanks