Re: [PATCH V4] exit: trigger panic when global init has exited
Hi, Eric, Oleg Any comment? >From the previous discussions, i think this change is necessary, but we need to confirm that move the decrement of signal->live is a safe.Here are some of my considerations There are three places that are going to be called besides do_exit(). 1. current_is_single_threaded() current_is_single_threaded() is used to check current process just has a single thread,my patch just moved the "signal->live--" position,this won't change anything,current_is_single_threaded() maybe get different value, after my patch,there is no change from the current logic. 2.css_task_iter_advance() Same as above,css_task_iter_advance() just read "signal->live",this may return different value,but it same before my patch. css_task_iter_advance() cgroup_threadgroup_change_begin() held around setting PF_EXITING before signal->live is decremented, cgroup_threadgroup_rwsem(cgroup_threadgroup_change_begin()) is used for user to get expect stable threadgroup,cgroup has no dependencies on setting PF_EXITING or signal->live decrement. 3.copy_process() copy_process() is called by fork(),copy_process will incremental "signal->live",signal->live is atomic operation,there is no race, the patch only move position,i don't see any new dependency problems Moving the decrement position mainly changes the order in which variables are assigned,we need to check if the change in the order of assignment has any side effects on other callers. i think acct_update_integrals(),sync_mm_rss() mainly updated some data,only exit_signals() and sched_exit() need attention. cgroup_threadgroup_change_begin() is called in exit_signals(),and css_task_iter_advance used "signal->live",it seems like it might be a little related. cgroup_threadgroup_change_begin() just give stable threadgroup for user,and css_task_iter_advance only check if group is dead, decrement of signal->live and sets PF_EXITING seems like safe. >From my current analysis, this is safe.
[PATCH V4] exit: trigger panic when global init has exited
From: Qianli Zhao
When init sub-threads running on different CPUs exit at the same time,
zap_pid_ns_processe()->BUG() may be happened(timing is as below),move
panic() before set PF_EXITING to fix this problem.
In addition,if panic() after other sub-threads finish do_exit(),
some key variables (task->mm,task->nsproxy etc) of sub-thread will be lost,
which makes it difficult to parse coredump from fulldump,checking
SIGNAL_GROUP_EXIT
to prevent init sub-threads exit.
[ 24.705376] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x7f00
[ 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
---
V4:
- Changelog update
V3:
- Use group_dead instead of thread_group_empty() to test single init exit.
V2:
- Changelog update
- Remove wrong useage of SIGNAL_UNKILLABLE.
- Add thread_group_empty() test to handle single init thread exit
---
kernel/exit.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e3..f95f8dc 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -766,6 +766,17 @@ void __noreturn do_exit(long code)
validate_creds_for_do_exit(tsk);
+ group_dead = atomic_dec_and_test(>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);
+ }
+
/*
* We're taking recursive faults here in do_exit. Safest is to just
* leave this task alone and wait for reboot.
@@ -784,16 +795,8 @@ void __noreturn do_exit(long code)
if (tsk->mm)
sync_mm_rss(tsk->mm);
acct_update_integrals(tsk);
- group_dead = atomic_dec_and_test(>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);
+ if (group_dead) {
#ifdef CONFIG_POSIX_TIMERS
hrtimer_cancel(>signal->real_timer);
exit_itimers(tsk->signal);
--
1.9.1
[PATCH V4] exit: trigger panic when global init has exited
From: Qianli Zhao
When init sub-threads running on different CPUs exit at the same time,
zap_pid_ns_processe()->BUG() may be happened(timing is as below),move
panic() before set PF_EXITING to fix this problem.
In addition,if panic() after other sub-threads finish do_exit(),
some key variables (task->mm,task->nsproxy etc) of sub-thread will be lost,
which makes it difficult to parse coredump from fulldump,checking
SIGNAL_GROUP_EXIT
to prevent init sub-threads exit.
[ 24.705376] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x7f00
[ 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
---
V4:
- Changelog update
V3:
- Use group_dead instead of thread_group_empty() to test single init exit.
V2:
- Changelog update
- Remove wrong useage of SIGNAL_UNKILLABLE.
- Add thread_group_empty() test to handle single init thread exit
---
kernel/exit.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e3..f95f8dc 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -766,6 +766,17 @@ void __noreturn do_exit(long code)
validate_creds_for_do_exit(tsk);
+ group_dead = atomic_dec_and_test(>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);
+ }
+
/*
* We're taking recursive faults here in do_exit. Safest is to just
* leave this task alone and wait for reboot.
@@ -784,16 +795,8 @@ void __noreturn do_exit(long code)
if (tsk->mm)
sync_mm_rss(tsk->mm);
acct_update_integrals(tsk);
- group_dead = atomic_dec_and_test(>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);
+ if (group_dead) {
#ifdef CONFIG_POSIX_TIMERS
hrtimer_cancel(>signal->real_timer);
exit_itimers(tsk->signal);
--
1.9.1
Re: [PATCH V3] exit: trigger panic when global init has exited
>> But,my patch has another purpose,protect some key variables(such >> as:task->mm,task->nsproxy,etc) to recover init coredump from >> fulldump,if sub-threads finish do_exit(), > Yes I know. > But the purpose of this SIGNAL_GROUP_EXIT check is not clear and not > documented. That is why I said it should be documented at least in the > changelog. Ok. I will update the changelog as you suggest. Oleg Nesterov 于2021年3月25日周四 上午2:12写道： > > Hi, > > On 03/23, qianli zhao wrote: > > > > Hi,Oleg > > > > > You certainly don't understand me :/ > > > > > Please read my email you quoted below. I didn't mean the current logic. > > > I meant the logic after your patch which moves atomic_dec_and_test() and > > > panic() before exit_signals(). > > > > Sorry, I think I see what you mean now. > > > > You mean that after apply my patch,SIGNAL_GROUP_EXIT no longer needs > > to be tested or avoid zap_pid_ns_processes()->BUG(). > > Yes,your consideration is correct. > > OK, great > > > But,my patch has another purpose,protect some key variables(such > > as:task->mm,task->nsproxy,etc) to recover init coredump from > > fulldump,if sub-threads finish do_exit(), > > Yes I know. > > But the purpose of this SIGNAL_GROUP_EXIT check is not clear and not > documented. That is why I said it should be documented at least in the > changelog. > > Oleg. >
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Oleg
> You certainly don't understand me :/
> Please read my email you quoted below. I didn't mean the current logic.
> I meant the logic after your patch which moves atomic_dec_and_test() and
> panic() before exit_signals().
Sorry, I think I see what you mean now.
You mean that after apply my patch,SIGNAL_GROUP_EXIT no longer needs
to be tested or avoid zap_pid_ns_processes()->BUG().
Yes,your consideration is correct.
But,my patch has another purpose,protect some key variables(such
as:task->mm,task->nsproxy,etc) to recover init coredump from
fulldump,if sub-threads finish do_exit(),these variables of sub-task
will be lost,and we cannot parse the coredump of the init process
through the tool normally such as "gcore".
Oleg Nesterov 于2021年3月23日周二 下午5:00写道：
>
> On 03/23, qianli zhao wrote:
> >
> > Hi,Oleg
> >
> > > No, there is at least one alive init thread. If they all have exited, we
> > > have
> > > the thread which calls panic() above.
> >
> > By current logic, setting PF_EXITING(exit_signals()) is before the
> > panic(),
>
> You certainly don't understand me :/
>
> Please read my email you quoted below. I didn't mean the current logic.
> I meant the logic after your patch which moves atomic_dec_and_test() and
> panic() before exit_signals().
>
> Oleg.
>
> > find_alive_thread() determines the PF_EXITING of all child
> > threads, the panic thread's PF_EXITING has been set before panic(),so
> > find_alive_thread() thinks this thread also dead, resulting in
> > find_alive_thread returning NULL.It is possible to trigger a
> > zap_pid_ns_processes()->BUG() in this case.
> >
> > exit_signals(tsk); /* sets PF_EXITING */
> > ...
> > group_dead = atomic_dec_and_test(>signal->live);
> > if (group_dead) {
> > if (unlikely(is_global_init(tsk)))
> > panic("Attempted to kill init!
> > exitcode=0x%08x\n",>//PF_EXITING has been set
> > tsk->signal->group_exit_code ?: (int)code);
> >
> > ===
> >
> > > Why do you think so? It can affect _any_ code which runs under
> > > "if (group_dead)". Again, I don't see anything wrong, but I didn't even
> > > try to audit these code paths.
> >
> > Yes,all places where checked the "signal->live" may be affected,but
> > even before my changes, each program that checks "signal->live" may
> > get different state(group_dead or not), depending on the timing of the
> > caller,this situation will not change after my change.
> > After my patch,"signal->live--" and other variable are set in a
> > different order(such as signal->live and PF_EXITING),this can cause
> > abnormalities in the logic associated with these two variables,that is
> > my thinking.
> > Of course, check all the "signal->live--" path is definitely
> > necessary,it's just the case above that we need more attention.
> >
> > Thanks
> >
> > Oleg Nesterov 于2021年3月23日周二 上午12:37写道：
> > >
> > > Hi,
> > >
> > > It seems that we don't understand each other.
> > >
> > > If we move atomic_dec_and_test(signal->live) and do
> > >
> > > if (group_dead && is_global_init)
> > > panic(...);
> > >
> > >
> > > before setting PF_EXITING like your patch does, then
> > > zap_pid_ns_processes()
> > > simply won't be called.
> > >
> > > Because:
> > >
> > > On 03/21, qianli zhao wrote:
> > > >
> > > > Hi,Oleg
> > > >
> > > > > How? Perhaps I missed something again, but I don't think this is
> > > > > possible.
> > > >
> > > > > zap_pid_ns_processes() simply won't be called, find_child_reaper()
> > > > > will
> > > > > see the !PF_EXITING thread which calls panic().
> > > >
> > > > > So I think this should be documented somehow, at least in the
> > > > > changelog.
> > > >
> > > > This problem occurs when both two init threads enter the do_exit,
> > > > One of the init thread is syscall sys_exit_group,and set
> > > > SIGNAL_GROUP_EXIT
> > > > The other init thread perform ret_to_user()->get_signal() and found
> > > > SIGNAL_GROUP_EXIT is set,then do_group_exit()->do_exit(),since there
> > > > are no alive init threads it finally goes to
> > > > zap_pid_ns_processes()
> > >
> > > No, there is at least one alive init thread. If they all have exited, we
> > > have
> > > the thread which calls panic() above.
> > >
> > > > and BUG().
> > >
> > > so we don't need the SIGNAL_GROUP_EXIT check to avoid this BUG().
> > >
> > > What have I missed?
> > >
> > > Oleg.
> > >
> >
>
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Oleg
> No, there is at least one alive init thread. If they all have exited, we have
> the thread which calls panic() above.
By current logic, setting PF_EXITING(exit_signals()) is before the
panic(),find_alive_thread() determines the PF_EXITING of all child
threads, the panic thread's PF_EXITING has been set before panic(),so
find_alive_thread() thinks this thread also dead, resulting in
find_alive_thread returning NULL.It is possible to trigger a
zap_pid_ns_processes()->BUG() in this case.
exit_signals(tsk); /* sets PF_EXITING */
...
group_dead = atomic_dec_and_test(>signal->live);
if (group_dead) {
if (unlikely(is_global_init(tsk)))
panic("Attempted to kill init!
exitcode=0x%08x\n",>//PF_EXITING has been set
tsk->signal->group_exit_code ?: (int)code);
===
> Why do you think so? It can affect _any_ code which runs under
> "if (group_dead)". Again, I don't see anything wrong, but I didn't even
> try to audit these code paths.
Yes,all places where checked the "signal->live" may be affected,but
even before my changes, each program that checks "signal->live" may
get different state(group_dead or not), depending on the timing of the
caller,this situation will not change after my change.
After my patch,"signal->live--" and other variable are set in a
different order(such as signal->live and PF_EXITING),this can cause
abnormalities in the logic associated with these two variables,that is
my thinking.
Of course, check all the "signal->live--" path is definitely
necessary,it's just the case above that we need more attention.
Thanks
Oleg Nesterov 于2021年3月23日周二 上午12:37写道：
>
> Hi,
>
> It seems that we don't understand each other.
>
> If we move atomic_dec_and_test(signal->live) and do
>
> if (group_dead && is_global_init)
> panic(...);
>
>
> before setting PF_EXITING like your patch does, then zap_pid_ns_processes()
> simply won't be called.
>
> Because:
>
> On 03/21, qianli zhao wrote:
> >
> > Hi,Oleg
> >
> > > How? Perhaps I missed something again, but I don't think this is possible.
> >
> > > zap_pid_ns_processes() simply won't be called, find_child_reaper() will
> > > see the !PF_EXITING thread which calls panic().
> >
> > > So I think this should be documented somehow, at least in the changelog.
> >
> > This problem occurs when both two init threads enter the do_exit,
> > One of the init thread is syscall sys_exit_group,and set SIGNAL_GROUP_EXIT
> > The other init thread perform ret_to_user()->get_signal() and found
> > SIGNAL_GROUP_EXIT is set,then do_group_exit()->do_exit(),since there
> > are no alive init threads it finally goes to
> > zap_pid_ns_processes()
>
> No, there is at least one alive init thread. If they all have exited, we have
> the thread which calls panic() above.
>
> > and BUG().
>
> so we don't need the SIGNAL_GROUP_EXIT check to avoid this BUG().
>
> What have I missed?
>
> Oleg.
>
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Eric,Oleg > It is a bit tricky to tell if the movement of the decrement of > signal->live is safe It is hard to say whether it is safe or not,below are just a few of my thoughts > That affects current_is_single threaded > which is used by unshare, setns of the time namespace, and setting > the selinux part of creds. I think is ok in current_is_single_threaded,in check_unshare_flags() and selinux_setprocattr() change "signal->live--" position won't change the result,There is no other dependency change to this patch and these function. > The usage in kernel/cgroup/cgroup.c:css_task_iter_advance seems safe. > Hmm, Maybe not. Today cgroup_thread_change_begin is held around > setting PF_EXITING before signal->live is decremented. So there seem to > be some subtle cgroup dependencies. Moving the decrement position should only affect between new and old code position of movement of the decrement of signal->live.In this range,i think acct_update_integrals(),sync_mm_rss() mainly updated some data,only exit_signals() and sched_exit() need attention. cgroup_threadgroup_change_begin() is called in exit_signals(),and css_task_iter_advance used "signal->live",it seems like it might be a little related. cgroup_threadgroup_change_begin() just give stable threadgroup for user,and css_task_iter_advance only check if group is dead, decrement of signal->live and sets PF_EXITING seems like safe. > I think if we are going to move the decrement of signal->live that > should be it's own patch and be accompanied with a good description of > why it is safe instead of having the decrement of signal->live be there > as a side effect of another change. I'm not sure how to describe whether this move is safe or not,from my analysis, no side effects have been found. Would you like tell me how to prove that or give me some suggestion? Thanks Eric W. Biederman 于2021年3月19日周五 上午3:09写道： > > Oleg Nesterov writes: > > > On 03/18, qianli zhao wrote: > >> > >> Hi,Oleg > >> > >> Thank you for your reply. > >> > >> >> When init sub-threads running on different CPUs exit at the same time, > >> >> zap_pid_ns_processe()->BUG() may be happened. > >> > >> > and why do you think your patch can't prevent this? > >> > >> > Sorry, I must have missed something. But it seems to me that you are > >> > trying > >> > to fix the wrong problem. Yes, zap_pid_ns_processes() must not be called > >> > in > >> > the root namespace, and this has nothing to do with CONFIG_PID_NS. > >> > >> Yes, i try to fix this exception by test SIGNAL_GROUP_EXIT and call > >> panic before setting PF_EXITING to prevent zap_pid_ns_processes() > >> being called when init do_exit(). > > > > Ah, I didn't notice your patch does atomic_dec_and_test(signal->live) > > before exit_signals() which sets PF_EXITING. Thanks for correcting me. > > > > So yes, I was wrong, your patch can prevent this. Although I'd like to > > recheck if every do-something-if-group-dead action is correct in the > > case we have a non-PF_EXITING thread... > > > > But then I don't understand the SIGNAL_GROUP_EXIT check added by your > > patch. Do we really need it if we want to avoid zap_pid_ns_processes() > > when the global init exits? > > > >> In addition, the patch also protects the init process state to > >> successfully get usable init coredump. > > > > Could you spell please? > > > > Does this connect to SIGNAL_GROUP_EXIT check? Do you mean that you want > > to panic earlier, before other init's sub-threads exit? > > That is my understanding. > > As I understand it this patch has two purposes: > 1. Avoid the BUG_ON in zap_pid_ns_processes when !CONFIG_PID_NS > 2. panic as early as possible so exiting threads don't removing >interesting debugging state. > > > It is a bit tricky to tell if the movement of the decrement of > signal->live is safe. That affects current_is_single threaded > which is used by unshare, setns of the time namespace, and setting > the selinux part of creds. > > The usage in kernel/cgroup/cgroup.c:css_task_iter_advance seems safe. > Hmm, Maybe not. Today cgroup_thread_change_begin is held around > setting PF_EXITING before signal->live is decremented. So there seem to > be some subtle cgroup dependencies. > > The usages of group_dead in do_exit seem safe, as except for the new > one everything is the same. > > We could definitely take advantage of knowing group_dead in exit_signals > to simplify it's optimization to not rerouting signals to living > threads. > > > I think if we are going to move the decrement of signal->live that > should be it's own patch and be accompanied with a good description of > why it is safe instead of having the decrement of signal->live be there > as a side effect of another change. > > Eric
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Oleg
> How? Perhaps I missed something again, but I don't think this is possible.
> zap_pid_ns_processes() simply won't be called, find_child_reaper() will
> see the !PF_EXITING thread which calls panic().
> So I think this should be documented somehow, at least in the changelog.
This problem occurs when both two init threads enter the do_exit,
One of the init thread is syscall sys_exit_group,and set SIGNAL_GROUP_EXIT
The other init thread perform ret_to_user()->get_signal() and found
SIGNAL_GROUP_EXIT is set,then do_group_exit()->do_exit(),since there
are no alive init threads it finally goes to
zap_pid_ns_processes() and BUG().
Please refer to the sequence diagram below(that has been written into
the changelog),and callstack is also below:
kernel log:
[ 24.705376] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x7f00
[ 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: 1 TASK: ffc973126900 CPU: 7 COMMAND: "init"
#0 [ff800805ba60] perf_trace_kernel_panic_late at ff99ac0bcbcc
#1 [ff800805bac0] die at ff99ac08dc64
#2 [ff800805bb10] bug_handler at ff99ac08e398
#3 [ff800805bbc0] brk_handler at ff99ac08529c
#4 [ff800805bc80] do_debug_exception at ff99ac0814e4
->exception
/home/work/courbet-r-stable-build/kernel/msm-4.14/include/linux/pid_namespace.h:
98
96static inline void zap_pid_ns_processes(struct pid_namespace *ns)
97{
98 BUG();
99}
#5 [ff800805bdf0] el1_dbg at ff99ac083298
#6 [ff800805be20] do_exit at ff99ac0c22e8
#7 [ff800805be80] do_group_exit at ff99ac0c2658
#8 [ff800805beb0] sys_exit_group at ff99ac0c266c
#9 [ff800805bff0] el0_svc_naked at ff99ac083cfc
PID: 552TASK: ffc9613c8f00 CPU: 4 COMMAND: "init"
#0 [ff801455b870] __delay at ff99ad32cc14
#1 [ff801455b8b0] __const_udelay at ff99ad32cd10
#2 [ff801455b8c0] msm_trigger_wdog_bite at ff99ac5d5be0
#3 [ff801455b980] do_msm_restart at ff99a3f8
#4 [ff801455b9b0] machine_restart at ff99ac085dd0
#5 [ff801455b9d0] emergency_restart at ff99ac0eb6dc
#6 [ff801455baf0] panic at ff99ac0bd008
> /home/work/courbet-r-stable-build/kernel/msm-4.14/kernel/exit.c:
842
836 if (group_dead) {
837 /*
838 * If the last thread of global init has exited, panic
839 * immediately to get a useable coredump.
840 */
841 if (unlikely(is_global_init(tsk)))
842 panic("Attempted to kill init! exitcode=0x%08x\n",
843 tsk->signal->group_exit_code ?: (int)code);
#7 [ff801455bb70] do_exit at ff99ac0c257c
#8 [ff801455bbd0] do_group_exit at ff99ac0c2644
#9 [ff801455bcc0] get_signal at ff99ac0d1384
#10 [ff801455be60] do_notify_resume at ff99ac08b2a8
#11 [ff801455bff0] work_pending at ff99ac083b8c
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()
Oleg Nesterov 于2021年3月20日周六 上午12:32写道：
>
> On 03/19, qianli zhao wrote:
> >
> > > But then I don't understand the SIGNAL_GROUP_EXIT check added by your
> > > patch. Do we really need it if we want to avoid zap_pid_ns_processes()
> > > when the global init exits?
> >
> > I think check SIGNAL_GROUP_EXIT is necessary,or panic() will happen
> > after all init sub-threads do_exit(),so the following two situations
> > will happen:
> > 1.According to the timing in the changelog,
> > zap_pid_ns_processes()->BUG() maybe happened.
>
> How? Perhaps I missed something again, but I don't think this is possible.
>
> zap_pid_ns_processes() simply won't be called, find_child_reaper() will
> see the !PF_EXITING thread which calls panic().
>
> So I think this should be documented somehow, at least in the changelog.
>
> Oleg.
>
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Eric
> As I understand it this patch has two purposes:
> 1. Avoid the BUG_ON in zap_pid_ns_processes when !CONFIG_PID_NS
> 2. panic as early as possible so exiting threads don't removing
> interesting debugging state.
Your understanding is very correct,this is what my patch wants to do
> I think if we are going to move the decrement of signal->live that
> should be it's own patch and be accompanied with a good description of
> why it is safe instead of having the decrement of signal->live be there
> as a side effect of another change.
I will think about the risks of movement of the decrement of
signal->live before exit_signal().
If is difficult to judge movement of the decrement of signal->live is
safe,how about only test 'signal->live==1' not use group_dead?
Such as:
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e3..87f3595 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) &&
+ ((atomic_read(>signal->live) == 1) ||/*current is
last init thread*/
+(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.
*/
@@ -784,16 +795,9 @@ void __noreturn do_exit(long code)
if (tsk->mm)
sync_mm_rss(tsk->mm);
acct_update_integrals(tsk);
+
group_dead = atomic_dec_and_test(>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);
-
Eric W. Biederman 于2021年3月19日周五 上午3:09写道：
>
> Oleg Nesterov writes:
>
> > On 03/18, qianli zhao wrote:
> >>
> >> Hi,Oleg
> >>
> >> Thank you for your reply.
> >>
> >> >> When init sub-threads running on different CPUs exit at the same time,
> >> >> zap_pid_ns_processe()->BUG() may be happened.
> >>
> >> > and why do you think your patch can't prevent this?
> >>
> >> > Sorry, I must have missed something. But it seems to me that you are
> >> > trying
> >> > to fix the wrong problem. Yes, zap_pid_ns_processes() must not be called
> >> > in
> >> > the root namespace, and this has nothing to do with CONFIG_PID_NS.
> >>
> >> Yes, i try to fix this exception by test SIGNAL_GROUP_EXIT and call
> >> panic before setting PF_EXITING to prevent zap_pid_ns_processes()
> >> being called when init do_exit().
> >
> > Ah, I didn't notice your patch does atomic_dec_and_test(signal->live)
> > before exit_signals() which sets PF_EXITING. Thanks for correcting me.
> >
> > So yes, I was wrong, your patch can prevent this. Although I'd like to
> > recheck if every do-something-if-group-dead action is correct in the
> > case we have a non-PF_EXITING thread...
> >
> > But then I don't understand the SIGNAL_GROUP_EXIT check added by your
> > patch. Do we really need it if we want to avoid zap_pid_ns_processes()
> > when the global init exits?
> >
> >> In addition, the patch also protects the init process state to
> >> successfully get usable init coredump.
> >
> > Could you spell please?
> >
> > Does this connect to SIGNAL_GROUP_EXIT check? Do you mean that you want
> > to panic earlier, before other init's sub-threads exit?
>
> That is my understanding.
>
> As I understand it this patch has two purposes:
> 1. Avoid the BUG_ON in zap_pid_ns_processes when !CONFIG_PID_NS
> 2. panic as early as possible so exiting threads don't removing
>interesting debugging state.
>
>
> It is a bit tricky to tell if the movement of the decrement of
> signal->live is safe. That affects current_is_single threaded
> which is used by unshare, setns of the time namespace, and setting
> the selinux part of creds.
>
> The usage in kernel/cgroup/cgroup.c:css_task_iter_advance seems safe.
> Hmm, Maybe not. Today cgroup_thread_change_begin is held around
> setting
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Oleg > But then I don't understand the SIGNAL_GROUP_EXIT check added by your > patch. Do we really need it if we want to avoid zap_pid_ns_processes() > when the global init exits? I think check SIGNAL_GROUP_EXIT is necessary,or panic() will happen after all init sub-threads do_exit(),so the following two situations will happen: 1.According to the timing in the changelog, zap_pid_ns_processes()->BUG() maybe happened. 2.The key variables of each init sub-threads will be in the exit state(such task->mm=NULL,task->flags=PF_EXITING,task->nsproxy=NULL),resulting in the failure to parse coredump from fulldump. So i think check SIGNAL_GROUP_EXIT is a simple and effective way to prevent these > Does this connect to SIGNAL_GROUP_EXIT check? Do you mean that you want > to panic earlier, before other init's sub-threads exit? Yes, my patch just want panic earlier before other init's sub-threads exit Oleg Nesterov 于2021年3月19日周五 上午2:05写道： > > On 03/18, qianli zhao wrote: > > > > Hi,Oleg > > > > Thank you for your reply. > > > > >> When init sub-threads running on different CPUs exit at the same time, > > >> zap_pid_ns_processe()->BUG() may be happened. > > > > > and why do you think your patch can't prevent this? > > > > > Sorry, I must have missed something. But it seems to me that you are > > > trying > > > to fix the wrong problem. Yes, zap_pid_ns_processes() must not be called > > > in > > > the root namespace, and this has nothing to do with CONFIG_PID_NS. > > > > Yes, i try to fix this exception by test SIGNAL_GROUP_EXIT and call > > panic before setting PF_EXITING to prevent zap_pid_ns_processes() > > being called when init do_exit(). > > Ah, I didn't notice your patch does atomic_dec_and_test(signal->live) > before exit_signals() which sets PF_EXITING. Thanks for correcting me. > > So yes, I was wrong, your patch can prevent this. Although I'd like to > recheck if every do-something-if-group-dead action is correct in the > case we have a non-PF_EXITING thread... > > But then I don't understand the SIGNAL_GROUP_EXIT check added by your > patch. Do we really need it if we want to avoid zap_pid_ns_processes() > when the global init exits? > > > In addition, the patch also protects the init process state to > > successfully get usable init coredump. > > Could you spell please? > > Does this connect to SIGNAL_GROUP_EXIT check? Do you mean that you want > to panic earlier, before other init's sub-threads exit? > > Thanks, > > Oleg. >
Re: [PATCH V3] exit: trigger panic when global init has exited
Hi,Oleg
Thank you for your reply.
>> When init sub-threads running on different CPUs exit at the same time,
>> zap_pid_ns_processe()->BUG() may be happened.
> and why do you think your patch can't prevent this?
> Sorry, I must have missed something. But it seems to me that you are trying
> to fix the wrong problem. Yes, zap_pid_ns_processes() must not be called in
> the root namespace, and this has nothing to do with CONFIG_PID_NS.
Yes, i try to fix this exception by test SIGNAL_GROUP_EXIT and call
panic before setting PF_EXITING to prevent zap_pid_ns_processes()
being called when init do_exit().
In addition, the patch also protects the init process state to
successfully get usable init coredump.
In my test,this patch works.
Oleg Nesterov 于2021年3月17日周三 下午10:38写道：
>
> On 03/17, Qianli Zhao wrote:
> >
> > From: Qianli Zhao
> >
> > When init sub-threads running on different CPUs exit at the same time,
> > zap_pid_ns_processe()->BUG() may be happened.
>
> and why do you think your patch can't prevent this?
>
> Sorry, I must have missed something. But it seems to me that you are trying
> to fix the wrong problem. Yes, zap_pid_ns_processes() must not be called in
> the root namespace, and this has nothing to do with CONFIG_PID_NS.
>
> > 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=0x7f00
> > [ 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
> > ---
> > V3:
> > - Use group_dead instead of thread_group_empty() to test single init exit.
> >
> > V2:
> > - Changelog update
> > - Remove wrong useage of SIGNAL_UNKILLABLE.
> > - Add thread_group_empty() test to handle single init thread exit
> > ---
> > kernel/exit.c | 20 +++-
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index 04029e3..32b74e4 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -766,6 +766,17 @@ void __noreturn do_exit(long code)
> >
> > validate_creds_for_do_exit(tsk);
> >
> > + group_dead = atomic_dec_and_test(>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);
> > + }
> > +
> > /*
> >* We're taking recursive faults here in do_exit. Safest is to just
> >* leave this task alone and wait for reboot.
> > @@ -784,16 +795,7 @@ void __noreturn do_exit(long code)
> > if (tsk->mm)
> > sync_mm_rss(tsk->mm);
> > acct_update_integrals(tsk);
> > - group_dead = atomic_dec_and_test(>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(>signal->real_timer);
> > exit_itimers(tsk->signal);
> > --
> > 1.9.1
> >
>
[PATCH V3] exit: trigger panic when global init has exited
From: Qianli Zhao
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=0x7f00
[ 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
---
V3:
- Use group_dead instead of thread_group_empty() to test single init exit.
V2:
- Changelog update
- Remove wrong useage of SIGNAL_UNKILLABLE.
- Add thread_group_empty() test to handle single init thread exit
---
kernel/exit.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e3..32b74e4 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -766,6 +766,17 @@ void __noreturn do_exit(long code)
validate_creds_for_do_exit(tsk);
+ group_dead = atomic_dec_and_test(>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);
+ }
+
/*
* We're taking recursive faults here in do_exit. Safest is to just
* leave this task alone and wait for reboot.
@@ -784,16 +795,7 @@ void __noreturn do_exit(long code)
if (tsk->mm)
sync_mm_rss(tsk->mm);
acct_update_integrals(tsk);
- group_dead = atomic_dec_and_test(>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(>signal->real_timer);
exit_itimers(tsk->signal);
--
1.9.1
Re: [PATCH V2] exit: trigger panic when global init has exited
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(>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(>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 AThread 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(>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(>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
[PATCH V2] exit: trigger panic when global init has exited
From: Qianli Zhao
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=0x7f00
[ 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
---
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(>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(>signal->real_timer);
exit_itimers(tsk->signal);
--
1.9.1
Re: [PATCH] exit: trigger panic when init process is set to SIGNAL_GROUP_EXIT
Hi, Eric
Thank you for your suggestion
> At the start of your changelog and your patch subject you describe what
> you are doing but not why. For the next revision of the patch please
> lead with the why it makes what you are trying to do much easier to
> understand.
got it.
>
> It does not work to use SIGNAL_UNKILLABLE for this. Normally init
> has SIGNAL_UNKILLABLE set. The only case that clears SIGNAL_UNKILLABLE
> is force_sig_info_to_task. If the init process exits with exit(2)
> SIGNAL_UNKILLABLE will already be set. Which means testing
> SIGNAL_UNKILLABLE as your patch does will prevent the panic.
>
Ok,using SIGNAL_UNKILLABLE is incorrect.
> Further simply calling panic is sufficient to guarantee that the other
> threads don't exit, and that whichever thread calls panic first
> will be the reporting thread. The rest of the threads will be caught
> in panic_smp_self_stop(), if they happen to be running on other cpus.
>
> So I would make the whole thing just be:
>
> /* 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);
> }
>
> The thread_group_empty test is needed to handle single threaded
> inits.
>
> Do you think you can respin your patch as something like that?
>
Ok.it's a very good change,other CPUs calls to panic() will be caught
and execute panic_smp_self_stop(),
there is no need to deal with this situation separately when other CPUs exit().
Re: [PATCH] exit: trigger panic when init process is set to SIGNAL_GROUP_EXIT
Hi,Oleg
Thanks for your replay.
> To be honest, I don't understand the changelog. It seems that you want
> to uglify the kernel to simplify the debugging of buggy init? Or what?
My patch is for the following purpose:
1. I hope to fix the occurrence of unexpected panic
- [ 24.705390] kernel BUG at include/linux/pid_namespace.h:98!
This problem occurs when both two init threads enter the do_exit,
One of the init thread is syscall sys_exit_group,and set SIGNAL_GROUP_EXIT
The other init thread perform ret_to_user()->get_signal() and found
SIGNAL_GROUP_EXIT is set,then do_group_exit()->do_exit()
Since there are no alive init threads it finally goes to
zap_pid_ns_processes() and BUG().
Timing details are in the changelog.
2. I hope to fix the problem that coredump cannot be parsed after init crash
Before my patch,ever init sub-thread will finish do_exit(),the last
thread will trigger panic().
Except for the thread that triggered the panic,the state(such as
PF_EXITING etc) of the other threads is already exiting,so we can't
parse coredump from fulldump
In fact, when any one init has been set to SIGNAL_GROUP_EXIT, then we
can trigger panic,and prevent other init threads from continuing to
exit
> Nor can I understand the patch. I fail to understand the games with
> SIGNAL_UNKILLABLE and ->siglock.
When first init thread panic,i don't want other init threads to still
exit,this will destroy the state of other init threads.
so i use SIGNAL_UNKILLABLE to mark this stat,prevent other init
threads from continuing to exit
In addition i use siglock to protect tsk->signal->flags.
> And iiuc with this patch the kernel will crash if init's sub-thread execs,
> signal_group_exit() returns T in this case.
Oleg Nesterov 于2021年3月10日周三 上午2:27写道：
>
> On 03/09, Qianli Zhao wrote:
> >
> > From: Qianli Zhao
> >
> > Once any init thread finds SIGNAL_GROUP_EXIT, trigger panic immediately
> > instead of last thread of global init has exited, and do not allow other
> > init threads to exit, protect task/memory state of all sub-threads for
> > get reliable init coredump
>
> To be honest, I don't understand the changelog. It seems that you want
> to uglify the kernel to simplify the debugging of buggy init? Or what?
>
> Nor can I understand the patch. I fail to understand the games with
> SIGNAL_UNKILLABLE and ->siglock.
>
> And iiuc with this patch the kernel will crash if init's sub-thread execs,
> signal_group_exit() returns T in this case.
>
> Oleg.
>
> > [ 24.705376] Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x7f00
> > [ 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
> > ---
> > We got an init crash issue, but we can't get init coredump from fulldump,
> > we also
> > see BUG() triggered which calling in zap_pid_ns_processes().
> >
> > From crash dump we can get the following information:
> > 1. "Attempted to kill init",init process is killed.
> > - Kernel panic - not syncing: Attempted to kill init! exitcode=0x7f00
> > 2. At the same time as init crash, a BUG() triggered in other core.
> > - [ 24.705390] kernel BUG at include/linux/pid_namespace.h:98!
> > 3. When init thread calls exit_mm, the corresponding thread task->mm will
> > be empty, which is not conducive to extracting coredump
> >
> > To fix the issue and save complete coredump, once find init thread is set
> > to SIGNAL_GROUP_EXIT
> > trigger panic immediately,and other child threads are not allowed
[PATCH] exit: trigger panic when init process is set to SIGNAL_GROUP_EXIT
From: Qianli Zhao
Once any init thread finds SIGNAL_GROUP_EXIT, trigger panic immediately
instead of last thread of global init has exited, and do not allow other
init threads to exit, protect task/memory state of all sub-threads for
get reliable init coredump
[ 24.705376] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x7f00
[ 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
---
We got an init crash issue, but we can't get init coredump from fulldump, we
also
see BUG() triggered which calling in zap_pid_ns_processes().
>From crash dump we can get the following information:
1. "Attempted to kill init",init process is killed.
- Kernel panic - not syncing: Attempted to kill init! exitcode=0x7f00
2. At the same time as init crash, a BUG() triggered in other core.
- [ 24.705390] kernel BUG at include/linux/pid_namespace.h:98!
3. When init thread calls exit_mm, the corresponding thread task->mm will be
empty, which is not conducive to extracting coredump
To fix the issue and save complete coredump, once find init thread is set to
SIGNAL_GROUP_EXIT
trigger panic immediately,and other child threads are not allowed to exit just
wait for reboot
PID: 1 TASK: ffc973126900 CPU: 7 COMMAND: "init"
#0 [ff800805ba60] perf_trace_kernel_panic_late at ff99ac0bcbcc
#1 [ff800805bac0] die at ff99ac08dc64
#2 [ff800805bb10] bug_handler at ff99ac08e398
#3 [ff800805bbc0] brk_handler at ff99ac08529c
#4 [ff800805bc80] do_debug_exception at ff99ac0814e4
#5 [ff800805bdf0] el1_dbg at ff99ac083298
->Exception
/home/work/courbet-r-stable-build/kernel/msm-4.14/include/linux/pid_namespace.h:
98
#6 [ff800805be20] do_exit at ff99ac0c22e8
#7 [ff800805be80] do_group_exit at ff99ac0c2658
#8 [ff800805beb0] sys_exit_group at ff99ac0c266c
#9 [ff800805bff0] el0_svc_naked at ff99ac083cf
->SYSCALLNO: 5e (__NR_exit_group)
PID: 552TASK: ffc9613c8f00 CPU: 4 COMMAND: "init"
#0 [ff801455b870] __delay at ff99ad32cc14
#1 [ff801455b8b0] __const_udelay at ff99ad32cd10
#2 [ff801455b8c0] msm_trigger_wdog_bite at ff99ac5d5be0
#3 [ff801455b980] do_msm_restart at ff99a3f8
#4 [ff801455b9b0] machine_restart at ff99ac085dd0
#5 [ff801455b9d0] emergency_restart at ff99ac0eb6dc
#6 [ff801455baf0] panic at ff99ac0bd008
#7 [ff801455bb70] do_exit at ff99ac0c257c
/home/work/courbet-r-stable-build/kernel/msm-4.14/kernel/exit.c: 842
#8 [ff801455bbd0] do_group_exit at ff99ac0c2644
#9 [ff801455bcc0] get_signal at ff99ac0d1384
#10 [ff801455be60] do_notify_resume at ff99ac08b2a8
#11 [ff801455bff0] work_pending at ff99ac083b8c
---
kernel/exit.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index ef2fb929..6b2da22 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -758,6 +758,27 @@ void __noreturn do_exit(long code)
validate_creds_for_do_exit(tsk);
/*
+* Once init group is marked for death,
+* panic immediately to get a useable coredump
+*/
+ if (unlikely(is_global_init(tsk) &&
+ signal_group_exit(tsk->signal))) {
+ spin_lock_irq(>sighand->siglock);
+ if (!(tsk->signal->flags & SIGNAL_UNKILLABLE)) {
+ tsk->signal->flags |= SIGNAL_UNKILLABLE;
+ spin_unlock_irq(>sighand->siglock);
+ panic("Attempted to kill init! exitcode=0x%08x\n",
+ tsk->signal->group_exit_code ?: (int)code);
+ } else {
+ /* init sub-thread is dying, just wait for reboot */
+ spin_unlock_irq(>
Re: [PATCH V6] kthread: Add debugobject support
Hi,tglx
Would you like to continue to review the new patch set?I made some
changes according to your suggestion.
Unless this change will not be considered or unnecessary.
Thanks
Qianli Zhao 于2020年10月12日周一 上午11:00写道：
>
> From: Qianli Zhao
>
> kthread_work is not covered by debug objects, but the same problems as with
> regular work objects apply.
>
> Some of the issues like reinitialization of an active kthread_work are hard
> to debug because the problem manifests itself later in a completely
> different context.
>
> Add debugobject support along with the necessary fixup functions to make
> debugging of these problems less tedious.
>
> Signed-off-by: Qianli Zhao
> ---
> V6:
> - Changelog update follow tglx's suggestion
> - Completion initialized as part of kthread_init_work_onstack() in
> kthread_flush_worker()/kthread_flush_work()
>
> V5:
> - Fix format error checked by checkpatch.pl
>
> V4:
> - Changelog update
> - Add comment for KWORK_ENTRY_STATIC
> - Code format modification
> - Check worker availability early in kthread_flush_work
>
> V3:
> - changelog update
> - add fixup_assert_init support
> - move debug_kwork_activate/debug_kwork_deactivate before list operation
> - name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
> - use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
> on stack
> - __init_kwork before clear kthread_work in kthread_init_work
> ---
> include/linux/kthread.h | 30 +-
> include/linux/poison.h | 4 ++
> kernel/kthread.c| 142
> +---
> lib/Kconfig.debug | 10
> 4 files changed, 176 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 65b81e0..706302b 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -108,6 +108,17 @@ struct kthread_delayed_work {
> struct timer_list timer;
> };
>
> +#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
> +extern void __init_kwork(struct kthread_work *kwork, int onstack);
> +extern void destroy_kwork_on_stack(struct kthread_work *kwork);
> +extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
> *kdwork);
> +#else
> +static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
> +static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
> +static inline void destroy_delayed_kwork_on_stack(struct
> kthread_delayed_work *kdwork) { }
> +#endif
> +
> +
> #define KTHREAD_WORKER_INIT(worker){ \
> .lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
> .work_list = LIST_HEAD_INIT((worker).work_list),\
> @@ -115,7 +126,7 @@ struct kthread_delayed_work {
> }
>
> #define KTHREAD_WORK_INIT(work, fn){ \
> - .node = LIST_HEAD_INIT((work).node),\
> + .node = { .next = KWORK_ENTRY_STATIC }, \
> .func = (fn), \
> }
>
> @@ -159,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
> *worker,
>
> #define kthread_init_work(work, fn)\
> do {\
> + __init_kwork(work, 0);
> \
> + memset((work), 0, sizeof(struct kthread_work)); \
> + INIT_LIST_HEAD(&(work)->node); \
> + (work)->func = (fn);\
> + } while (0)
> +
> +#define kthread_init_work_onstack(work, fn)
> \
> + do {\
> + __init_kwork(work, 1);
> \
> memset((work), 0, sizeof(struct kthread_work)); \
> INIT_LIST_HEAD(&(work)->node); \
> (work)->func = (fn);\
> @@ -172,6 +192,14 @@ extern void __kthread_init_worker(struct kthread_worker
> *worker,
> TIMER_IRQSAFE);\
> } while (0)
>
> +#define kthread_init_delayed_work_onstack(dwork, fn)
> \
> + do {\
> + kthread_init_work_onstack(&(dwork)->work, (fn));
> \
> + __init_timer_on_stack(&
[PATCH V6] kthread: Add debugobject support
From: Qianli Zhao
kthread_work is not covered by debug objects, but the same problems as with
regular work objects apply.
Some of the issues like reinitialization of an active kthread_work are hard
to debug because the problem manifests itself later in a completely
different context.
Add debugobject support along with the necessary fixup functions to make
debugging of these problems less tedious.
Signed-off-by: Qianli Zhao
---
V6:
- Changelog update follow tglx's suggestion
- Completion initialized as part of kthread_init_work_onstack() in
kthread_flush_worker()/kthread_flush_work()
V5:
- Fix format error checked by checkpatch.pl
V4:
- Changelog update
- Add comment for KWORK_ENTRY_STATIC
- Code format modification
- Check worker availability early in kthread_flush_work
V3:
- changelog update
- add fixup_assert_init support
- move debug_kwork_activate/debug_kwork_deactivate before list operation
- name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
- use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
on stack
- __init_kwork before clear kthread_work in kthread_init_work
---
include/linux/kthread.h | 30 +-
include/linux/poison.h | 4 ++
kernel/kthread.c| 142 +---
lib/Kconfig.debug | 10
4 files changed, 176 insertions(+), 10 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..706302b 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,17 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +126,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -159,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
+ __init_kwork(work, 0);
\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ __init_kwork(work, 1);
\
memset((work), 0, sizeof(struct kthread_work)); \
INIT_LIST_HEAD(&(work)->node); \
(work)->func = (fn);\
@@ -172,6 +192,14 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
TIMER_IRQSAFE);\
} while (0)
+#define kthread_init_delayed_work_onstack(dwork, fn)
\
+ do {\
+ kthread_init_work_onstack(&(dwork)->work, (fn));
\
+ __init_timer_on_stack(&(dwork)->timer,
\
+kthread_delayed_work_timer_fn, \
+TIMER_IRQSAFE);\
+ } while (0)
+
int kthread_worker_fn(void *worker_ptr);
__printf(2, 3)
diff --git a/include/linux/poison.h b/include/linux/poison.h
index dc8ae5d..50811c74 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -82,4 +82,8 @@
/** security/ **/
#define KEY_DESTROY0xbd
+/** kernel/kthread **/
+/*indicate a static kthread_work initializer for the object debugging code.*/
+#define KWORK_ENTRY_STATIC ((void *) 0x600 + POISON_POINTER_DELTA)
+
#endif
diff --git a/kernel/
Re: [PATCH v5] kthread: Add debugobject support
Hi,Thomas
Thanks for your reply
On Thu, 1 Oct 2020 at 22:34, Thomas Gleixner wrote:
>
> On Mon, Aug 17 2020 at 14:37, Qianli Zhao wrote:
> > From: Qianli Zhao
> >
> > Add debugobject support to track the life time of kthread_work
> > which is used to detect reinitialization/free active object problems
> > Add kthread_init_work_onstack()/kthread_init_delayed_work_onstack() for
> > kthread onstack support
> >
> > If we reinitialize a kthread_work that has been activated,
> > this will cause delayed_work_list/work_list corruption.
> > enable this config,there is an chance to fixup these errors
> > or WARNING the wrong use of kthread_work
> >
> > [30858.395766] list_del corruption. next->prev should be ffe388ebbf88,
> > but was ffe388ebb588
> > [30858.395788] WARNING: CPU: 2 PID: 387 at
> > kernel/msm-4.19/lib/list_debug.c:56 __list_del_entry_valid+0xc8/0xd0
> > ...
> > [30858.395906] Call trace:
> > [30858.395909] __list_del_entry_valid+0xc8/0xd0
> > [30858.395912] __kthread_cancel_work_sync+0x98/0x138
> > [30858.395915] kthread_cancel_delayed_work_sync+0x10/0x20
> > [30858.395917] sde_encoder_resource_control+0xe8/0x12c0
> > [30858.395920] sde_encoder_prepare_for_kickoff+0x5dc/0x2008
> > [30858.395923] sde_crtc_commit_kickoff+0x280/0x890
> > [30858.395925] sde_kms_commit+0x16c/0x278
> > [30858.395928] complete_commit+0x3c4/0x760
> > [30858.395931] _msm_drm_commit_work_cb+0xec/0x1e0
> > [30858.395933] kthread_worker_fn+0xf8/0x190
> > [30858.395935] kthread+0x118/0x128
> > [30858.395938] ret_from_fork+0x10/0x18
> >
> > crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
> > [ffe389392620] delayed_work_list = {
> > next = 0xffe388ebbf88,
> > prev = 0xffe388ebb588
> > }
> > crash> list 0xffe388ebbf88
> > ffe388ebbf88
>
> This changelog is confusing at best. Something like this perhaps?
>
> kthread_work is not covered by debug objects, but the same problems as with
> regular work objects apply.
>
> Some of the issues like reinitialization of an active kthread_work are hard
> to debug because the problem manifests itself later in a completely
> different context.
>
> Add debugobject support along with the necessary fixup functions to make
> debugging of these problems less tedious.
>
Will follow your tips to improve.
> > +static void stub_kthread_work(struct kthread_work *unuse)
>
> unused?
>
> > +{
> > + WARN_ON(1);
> > +}
When the kthread_work is not initialized, kwork_fixup_assert_init()
will call kthread_init_work() to fixup this kthread_work,and
kthread_init_work() needs a function as a parameter,so we have to
quote an empty function(refer to stub_timer()).
> > void kthread_flush_work(struct kthread_work *work)
> > {
> > struct kthread_flush_work fwork = {
> > - KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> > - COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > + .done = COMPLETION_INITIALIZER_ONSTACK(fwork.done),
>
> Eew. Why is the completion initialized seperately instead of being
> initialized as part of kthread_init_work_onstack() ?
>
I just try to keep the same as before,how about change as below?
completion initialized as part of kthread_init_work_onstack()
@@ -1319,10 +1318,9 @@ bool kthread_cancel_delayed_work_sync(struct
kthread_delayed_work *dwork)
*/
void kthread_flush_worker(struct kthread_worker *worker)
{
- struct kthread_flush_work fwork = {
- .done = COMPLETION_INITIALIZER_ONSTACK(fwork.done),
- };
+ struct kthread_flush_work fwork;
+ fwork.done = COMPLETION_INITIALIZER_ONSTACK(fwork.done);
kthread_init_work_onstack(, kthread_flush_work_fn);
> > };
> > struct kthread_worker *worker;
> > bool noop = false;
> >
> > + debug_kwork_assert_init(work);
> > worker = work->worker;
> > if (!worker)
> > return;
> >
> > + kthread_init_work_onstack(, kthread_flush_work_fn);
> >
> > @@ -1194,12 +1319,13 @@ EXPORT_SYMBOL_GPL(kthread_cancel_delayed_work_sync);
> > void kthread_flush_worker(struct kthread_worker *worker)
> > {
> > struct kthread_flush_work fwork = {
> > - KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> > - COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > + .done = COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > };
>
> Ditto.
>
> > + kthread_init_work_onstack(, kthread_flush_work_fn);
> > kthread_queue_work(worker, );
> > wait_for_completion();
> > + destroy_kwork_on_stack();
>
> Thanks,
>
> tglx
[tip: timers/core] timers: Mask invalid flags in do_init_timer()
The following commit has been merged into the timers/core branch of tip:
Commit-ID: b952caf2d5ca898cc10d63be7722ae7a5daca696
Gitweb:
https://git.kernel.org/tip/b952caf2d5ca898cc10d63be7722ae7a5daca696
Author:Qianli Zhao
AuthorDate:Thu, 13 Aug 2020 23:03:14 +08:00
Committer: Thomas Gleixner
CommitterDate: Thu, 24 Sep 2020 22:12:18 +02:00
timers: Mask invalid flags in do_init_timer()
do_init_timer() accepts any combination of timer flags handed in by the
caller without a sanity check, but only TIMER_DEFFERABLE, TIMER_PINNED and
TIMER_IRQSAFE are valid.
If the supplied flags have other bits set, this could result in
malfunction. If bits are set in TIMER_CPUMASK the first timer usage could
deference a cpu base which is outside the range of possible CPUs. If
TIMER_MIGRATION is set, then the switch_timer_base() will live lock.
Prevent that with a sanity check which warns when invalid flags are
supplied and masks them out.
[ tglx: Made it WARN_ON_ONCE() and added context to the changelog ]
Signed-off-by: Qianli Zhao
Signed-off-by: Thomas Gleixner
Link:
https://lore.kernel.org/r/9d79a8aa4eb56713af7379f99f062dedabcde140.1597326756.git.zhaoqia...@xiaomi.com
---
include/linux/timer.h | 1 +
kernel/time/timer.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/include/linux/timer.h b/include/linux/timer.h
index 07910ae..d10bc7e 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -67,6 +67,7 @@ struct timer_list {
#define TIMER_DEFERRABLE 0x0008
#define TIMER_PINNED 0x0010
#define TIMER_IRQSAFE 0x0020
+#define TIMER_INIT_FLAGS (TIMER_DEFERRABLE | TIMER_PINNED |
TIMER_IRQSAFE)
#define TIMER_ARRAYSHIFT 22
#define TIMER_ARRAYMASK0xFFC0
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index a16764b..25e048d 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -794,6 +794,8 @@ static void do_init_timer(struct timer_list *timer,
{
timer->entry.pprev = NULL;
timer->function = func;
+ if (WARN_ON_ONCE(flags & ~TIMER_INIT_FLAGS))
+ flags &= TIMER_INIT_FLAGS;
timer->flags = flags | raw_smp_processor_id();
lockdep_init_map(>lockdep_map, name, key, 0);
}
Re: [PATCH v5] kthread: Add debugobject support
Dear maintainer
Is this change ignored or rejected?
I'm not sure who is the maintainer of kthread, please help give me a
definite reply
Thanks
On Mon, 17 Aug 2020 at 14:37, Qianli Zhao wrote:
>
> From: Qianli Zhao
>
> Add debugobject support to track the life time of kthread_work
> which is used to detect reinitialization/free active object problems
> Add kthread_init_work_onstack()/kthread_init_delayed_work_onstack() for
> kthread onstack support
>
> If we reinitialize a kthread_work that has been activated,
> this will cause delayed_work_list/work_list corruption.
> enable this config,there is an chance to fixup these errors
> or WARNING the wrong use of kthread_work
>
> [30858.395766] list_del corruption. next->prev should be ffe388ebbf88,
> but was ffe388ebb588
> [30858.395788] WARNING: CPU: 2 PID: 387 at
> kernel/msm-4.19/lib/list_debug.c:56 __list_del_entry_valid+0xc8/0xd0
> ...
> [30858.395906] Call trace:
> [30858.395909] __list_del_entry_valid+0xc8/0xd0
> [30858.395912] __kthread_cancel_work_sync+0x98/0x138
> [30858.395915] kthread_cancel_delayed_work_sync+0x10/0x20
> [30858.395917] sde_encoder_resource_control+0xe8/0x12c0
> [30858.395920] sde_encoder_prepare_for_kickoff+0x5dc/0x2008
> [30858.395923] sde_crtc_commit_kickoff+0x280/0x890
> [30858.395925] sde_kms_commit+0x16c/0x278
> [30858.395928] complete_commit+0x3c4/0x760
> [30858.395931] _msm_drm_commit_work_cb+0xec/0x1e0
> [30858.395933] kthread_worker_fn+0xf8/0x190
> [30858.395935] kthread+0x118/0x128
> [30858.395938] ret_from_fork+0x10/0x18
>
> crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
> [ffe389392620] delayed_work_list = {
> next = 0xffe388ebbf88,
> prev = 0xffe388ebb588
> }
> crash> list 0xffe388ebbf88
> ffe388ebbf88
>
> Signed-off-by: Qianli Zhao
> ---
> V5:
> - Fix format error checked by checkpatch.pl
>
> V4:
> - Changelog update
> - Add comment for KWORK_ENTRY_STATIC
> - Code format modification
> - Check worker availability early in kthread_flush_work
>
> V3:
> - changelog update
> - add fixup_assert_init support
> - move debug_kwork_activate/debug_kwork_deactivate before list operation
> - name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
> - use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
> on stack
> - __init_kwork before clear kthread_work in kthread_init_work
> ---
> include/linux/kthread.h | 30 ++-
> include/linux/poison.h | 4 ++
> kernel/kthread.c| 136
> ++--
> lib/Kconfig.debug | 10
> 4 files changed, 174 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 65b81e0..706302b 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -108,6 +108,17 @@ struct kthread_delayed_work {
> struct timer_list timer;
> };
>
> +#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
> +extern void __init_kwork(struct kthread_work *kwork, int onstack);
> +extern void destroy_kwork_on_stack(struct kthread_work *kwork);
> +extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
> *kdwork);
> +#else
> +static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
> +static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
> +static inline void destroy_delayed_kwork_on_stack(struct
> kthread_delayed_work *kdwork) { }
> +#endif
> +
> +
> #define KTHREAD_WORKER_INIT(worker){ \
> .lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
> .work_list = LIST_HEAD_INIT((worker).work_list),\
> @@ -115,7 +126,7 @@ struct kthread_delayed_work {
> }
>
> #define KTHREAD_WORK_INIT(work, fn){ \
> - .node = LIST_HEAD_INIT((work).node),\
> + .node = { .next = KWORK_ENTRY_STATIC }, \
> .func = (fn), \
> }
>
> @@ -159,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
> *worker,
>
> #define kthread_init_work(work, fn)\
> do {\
> + __init_kwork(work, 0);
> \
> + memset((work), 0, sizeof(struct kthread_work)); \
> + INIT_LIST_HEAD(&(work)->node); \
> + (work)->func = (fn);
[PATCH v4] workqueue: Warn when work flush own workqueue
From: Qianli Zhao
If a workqueue flushes itself then that will lead to
a deadlock. Print a warning and a stack trace when
this happens.
crash> ps 10856
PIDPPID CPU TASK ST COMM
108562 2 ffc873428080 UN [kworker/u16:15]
crash> bt 10856
PID: 10856 TASK: ffc873428080 CPU: 2 COMMAND: "kworker/u16:15"
#0 [ff80270cb9a0] __switch_to at ff99bba8533c
#1 [ff80270cba30] __schedule at ff99bcda18dc
#2 [ff80270cba50] schedule at ff99bcda1cdc
#3 [ff80270cbaf0] schedule_timeout at ff99bcda6674
#4 [ff80270cbb70] wait_for_common at ff99bcda2c68
#5 [ff80270cbb80] wait_for_completion at ff99bcda2b60
#6 [ff80270cbc30] flush_workqueue at ff99bbad7a60
#7 [ff80270cbc90] drain_workqueue at ff99bbad80fc
#8 [ff80270cbcb0] destroy_workqueue at ff99bbad92f8
#9 [ff80270cbda0] dfc_svc_init at ff99bbfbfb6c
#10 [ff80270cbdf0] process_one_work at ff99bbadc478
#11 [ff80270cbe50] worker_thread at ff99bbadc9dc
#12 [ff80270cbeb0] kthread at ff99bbae1f84
Reported-by: kernel test robot
Signed-off-by: Qianli Zhao
---
Changes in V4:
- Fix an error that when different work item used same workqueue in flush_work,
patch will trigger WARNING incorrectly, reported-by "kernel test robot
"
Changes in V3:
- update changelog
- update comment
Changes in V2:
- update changelog
- update comment
kernel/workqueue.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c41c3c1..6a8a4e0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2585,6 +2585,7 @@ static int rescuer_thread(void *__rescuer)
* @target_work: work item being flushed (NULL for workqueue flushes)
*
* %current is trying to flush the whole @target_wq or @target_work on it.
+ * If a work queue flushes itself, that will lead to a deadlock.
* If @target_wq doesn't have %WQ_MEM_RECLAIM, verify that %current is not
* reclaiming memory or running on a workqueue which doesn't have
* %WQ_MEM_RECLAIM as that can break forward-progress guarantee leading to
@@ -2594,13 +2595,17 @@ static void check_flush_dependency(struct
workqueue_struct *target_wq,
struct work_struct *target_work)
{
work_func_t target_func = target_work ? target_work->func : NULL;
- struct worker *worker;
+ struct worker *worker = current_wq_worker();
+
+ WARN_ONCE(worker && worker->current_pwq->wq == target_wq &&
+ worker->task == current &&
+ (target_work ? worker->current_work == target_work : true),
+ "workqueue: current work function:%ps is flushing own
workqueue:%s",
+ worker->current_func, target_wq->name);
if (target_wq->flags & WQ_MEM_RECLAIM)
return;
- worker = current_wq_worker();
-
WARN_ONCE(current->flags & PF_MEMALLOC,
"workqueue: PF_MEMALLOC task %d(%s) is flushing
!WQ_MEM_RECLAIM %s:%ps",
current->pid, current->comm, target_wq->name, target_func);
--
2.7.4
Re: [kthread] 2e7d8748eb: last_state.is_incomplete_run
I did not see any exceptions related to my changes,the corresponding
macro CONFIG_DEBUG_OBJECTS_KTHREAD is not enabled,so i think the issue
has nothing to do with my changes
Thanks
On Thu, 20 Aug 2020 at 14:26, kernel test robot wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 2e7d8748eba7e32150cbd4f57129ea77d1255892 ("[RFC V2] kthread: add
> object debug support")
> url:
> https://github.com/0day-ci/linux/commits/Qianli-Zhao/kthread-add-object-debug-support/20200812-131719
> base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git
> fb893de323e2d39f7a1f6df425703a2edbdf56ea
>
> in testcase: boot
>
> on test machine: 8 threads Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz with 16G
> memory
>
> caused below changes (please refer to attached dmesg/kmsg for entire
> log/backtrace):
>
>
>
> kernel boot failed by kexec:
>
> user :notice: [32m[ +0.371313] [0m [33mLKP [0m: kexec loading...
> user :notice: [32m[ +0.007118] [0mkexec --noefi -l
> /opt/rootfs/tmp/pkg/linux/x86_64-rhel-8.3/gcc-9/2e7d8748eba7e32150cbd4f57129ea77d1255892/vmlinuz-5.8.0-12610-g2e7d8748eba7e
> --initrd=/opt/rootfs/tmp/initrd-concatenated
>
>
>
>
> Thanks,
> Rong Chen
>
[PATCH v3] workqueue: Warn when work flush own workqueue
From: Qianli Zhao
If a workqueue flushes itself then that will lead to
a deadlock. Print a warning and a stack trace when
this happens.
crash> ps 10856
PIDPPID CPU TASK ST COMM
108562 2 ffc873428080 UN [kworker/u16:15]
crash> bt 10856
PID: 10856 TASK: ffc873428080 CPU: 2 COMMAND: "kworker/u16:15"
#0 [ff80270cb9a0] __switch_to at ff99bba8533c
#1 [ff80270cba30] __schedule at ff99bcda18dc
#2 [ff80270cba50] schedule at ff99bcda1cdc
#3 [ff80270cbaf0] schedule_timeout at ff99bcda6674
#4 [ff80270cbb70] wait_for_common at ff99bcda2c68
#5 [ff80270cbb80] wait_for_completion at ff99bcda2b60
#6 [ff80270cbc30] flush_workqueue at ff99bbad7a60
#7 [ff80270cbc90] drain_workqueue at ff99bbad80fc
#8 [ff80270cbcb0] destroy_workqueue at ff99bbad92f8
#9 [ff80270cbda0] dfc_svc_init at ff99bbfbfb6c
#10 [ff80270cbdf0] process_one_work at ff99bbadc478
#11 [ff80270cbe50] worker_thread at ff99bbadc9dc
#12 [ff80270cbeb0] kthread at ff99bbae1f84
Signed-off-by: Qianli Zhao
---
Changes in V3:
- update changelog
- update comment
Changes in V2:
- update changelog
- update comment
kernel/workqueue.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c41c3c1..a46d289 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2585,6 +2585,7 @@ static int rescuer_thread(void *__rescuer)
* @target_work: work item being flushed (NULL for workqueue flushes)
*
* %current is trying to flush the whole @target_wq or @target_work on it.
+ * If a work queue flushes itself, that will lead to a deadlock.
* If @target_wq doesn't have %WQ_MEM_RECLAIM, verify that %current is not
* reclaiming memory or running on a workqueue which doesn't have
* %WQ_MEM_RECLAIM as that can break forward-progress guarantee leading to
@@ -2594,13 +2595,16 @@ static void check_flush_dependency(struct
workqueue_struct *target_wq,
struct work_struct *target_work)
{
work_func_t target_func = target_work ? target_work->func : NULL;
- struct worker *worker;
+ struct worker *worker = current_wq_worker();
+
+ WARN_ONCE(worker && worker->current_pwq->wq == target_wq &&
+ worker->task == current,
+ "workqueue: current work function:%ps is flushing own
workqueue:%s",
+ worker->current_func, target_wq->name);
if (target_wq->flags & WQ_MEM_RECLAIM)
return;
- worker = current_wq_worker();
-
WARN_ONCE(current->flags & PF_MEMALLOC,
"workqueue: PF_MEMALLOC task %d(%s) is flushing
!WQ_MEM_RECLAIM %s:%ps",
current->pid, current->comm, target_wq->name, target_func);
--
2.7.4
Re: [PATCH v2] workqueue: Warn when work flush own workqueue
Markus Thanks for your suggestion,and sorry for my poor wording. On Tue, Aug 25, 2020 at 4:00 PM Markus Elfring wrote: > > > Flushing own workqueue or work self in work context will lead to > > a deadlock. > > I imagine that the wording “or work self” can become clearer another bit. > > > > Catch this incorrect usage and issue a warning when issue happened > > * Would you like to mark the end of such a sentence with a dot? > > * How do you think about to adjust the repetition of the word “issue”? How about below changelog? workqueue: Warn when work flush own workqueue Flushing itself or own workqueue in work context will lead to a deadlock. Catch this incorrect usage and warning when issue happened. > > > … > > - update comment > > --- > > kernel/workqueue.c | 10 +++--- > > I suggest to replace these triple dashes by a blank line. Ok > > > … > > @@ -2585,6 +2585,7 @@ static int rescuer_thread(void *__rescuer) > > * @target_work: work item being flushed (NULL for workqueue flushes) > > * > > * %current is trying to flush the whole @target_wq or @target_work on it. > > + * If a work flushing own workqueue or itself will lead to a deadlock. > > I stumble on understanding challenges for the wording “work flushing”. > Can an adjustment help in comparison to the term “work item”? How about below comment? * If a work item flushing own workqueue or itself will lead to a deadlock. > > Regards, > Markus
[PATCH v2] workqueue: Warn when work flush own workqueue
From: Qianli Zhao
Flushing own workqueue or work self in work context will lead to
a deadlock.
Catch this incorrect usage and issue a warning when issue happened
crash> ps 10856
PIDPPID CPU TASK ST COMM
108562 2 ffc873428080 UN [kworker/u16:15]
crash> bt 10856
PID: 10856 TASK: ffc873428080 CPU: 2 COMMAND: "kworker/u16:15"
#0 [ff80270cb9a0] __switch_to at ff99bba8533c
#1 [ff80270cba30] __schedule at ff99bcda18dc
#2 [ff80270cba50] schedule at ff99bcda1cdc
#3 [ff80270cbaf0] schedule_timeout at ff99bcda6674
#4 [ff80270cbb70] wait_for_common at ff99bcda2c68
#5 [ff80270cbb80] wait_for_completion at ff99bcda2b60
#6 [ff80270cbc30] flush_workqueue at ff99bbad7a60
#7 [ff80270cbc90] drain_workqueue at ff99bbad80fc
#8 [ff80270cbcb0] destroy_workqueue at ff99bbad92f8
#9 [ff80270cbda0] dfc_svc_init at ff99bbfbfb6c
#10 [ff80270cbdf0] process_one_work at ff99bbadc478
#11 [ff80270cbe50] worker_thread at ff99bbadc9dc
#12 [ff80270cbeb0] kthread at ff99bbae1f84
Signed-off-by: Qianli Zhao
---
Changes in V2:
- update changelog
- update comment
---
kernel/workqueue.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 3f5d4bf..9798d77 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2585,6 +2585,7 @@ static int rescuer_thread(void *__rescuer)
* @target_work: work item being flushed (NULL for workqueue flushes)
*
* %current is trying to flush the whole @target_wq or @target_work on it.
+ * If a work flushing own workqueue or itself will lead to a deadlock.
* If @target_wq doesn't have %WQ_MEM_RECLAIM, verify that %current is not
* reclaiming memory or running on a workqueue which doesn't have
* %WQ_MEM_RECLAIM as that can break forward-progress guarantee leading to
@@ -2594,13 +2595,16 @@ static void check_flush_dependency(struct
workqueue_struct *target_wq,
struct work_struct *target_work)
{
work_func_t target_func = target_work ? target_work->func : NULL;
- struct worker *worker;
+ struct worker *worker = current_wq_worker();
+
+ WARN_ONCE(worker && worker->current_pwq->wq == target_wq &&
+ worker->task == current,
+ "workqueue: current work function:%ps is flushing own
workqueue:%s",
+ worker->current_func, target_wq->name);
if (target_wq->flags & WQ_MEM_RECLAIM)
return;
- worker = current_wq_worker();
-
WARN_ONCE(current->flags & PF_MEMALLOC,
"workqueue: PF_MEMALLOC task %d(%s) is flushing
!WQ_MEM_RECLAIM %s:%ps",
current->pid, current->comm, target_wq->name, target_func);
--
2.7.4
[PATCH] workqueue: Warning when work try to flush own workqueue
From: Qianli Zhao
In a work process context,flush own workqueue or work self
will cause process blocked(enter state D),leading to a
deadlock,catch this wrong use,warn when the issue happened
crash> ps 10856
PIDPPID CPU TASK ST COMM
108562 2 ffc873428080 UN [kworker/u16:15]
crash> bt 10856
PID: 10856 TASK: ffc873428080 CPU: 2 COMMAND: "kworker/u16:15"
#0 [ff80270cb9a0] __switch_to at ff99bba8533c
#1 [ff80270cba30] __schedule at ff99bcda18dc
#2 [ff80270cba50] schedule at ff99bcda1cdc
#3 [ff80270cbaf0] schedule_timeout at ff99bcda6674
#4 [ff80270cbb70] wait_for_common at ff99bcda2c68
#5 [ff80270cbb80] wait_for_completion at ff99bcda2b60
#6 [ff80270cbc30] flush_workqueue at ff99bbad7a60
#7 [ff80270cbc90] drain_workqueue at ff99bbad80fc
#8 [ff80270cbcb0] destroy_workqueue at ff99bbad92f8
#9 [ff80270cbda0] dfc_svc_init at ff99bbfbfb6c
#10 [ff80270cbdf0] process_one_work at ff99bbadc478
#11 [ff80270cbe50] worker_thread at ff99bbadc9dc
#12 [ff80270cbeb0] kthread at ff99bbae1f84
Signed-off-by: Qianli Zhao
---
kernel/workqueue.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 3f5d4bf..759280d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2585,6 +2585,8 @@ static int rescuer_thread(void *__rescuer)
* @target_work: work item being flushed (NULL for workqueue flushes)
*
* %current is trying to flush the whole @target_wq or @target_work on it.
+ * If a work try to flush own workqueue or itself will cause process blocked,
+ * leading to a dealock.
* If @target_wq doesn't have %WQ_MEM_RECLAIM, verify that %current is not
* reclaiming memory or running on a workqueue which doesn't have
* %WQ_MEM_RECLAIM as that can break forward-progress guarantee leading to
@@ -2594,13 +2596,16 @@ static void check_flush_dependency(struct
workqueue_struct *target_wq,
struct work_struct *target_work)
{
work_func_t target_func = target_work ? target_work->func : NULL;
- struct worker *worker;
+ struct worker *worker = current_wq_worker();
+
+ WARN_ONCE(worker && worker->current_pwq->wq == target_wq &&
+ worker->task == current,
+ "workqueue: current work function:%ps is flushing own
workqueue:%s",
+ worker->current_func, target_wq->name);
if (target_wq->flags & WQ_MEM_RECLAIM)
return;
- worker = current_wq_worker();
-
WARN_ONCE(current->flags & PF_MEMALLOC,
"workqueue: PF_MEMALLOC task %d(%s) is flushing
!WQ_MEM_RECLAIM %s:%ps",
current->pid, current->comm, target_wq->name, target_func);
--
2.7.4
[PATCH v5] kthread: Add debugobject support
From: Qianli Zhao
Add debugobject support to track the life time of kthread_work
which is used to detect reinitialization/free active object problems
Add kthread_init_work_onstack()/kthread_init_delayed_work_onstack() for
kthread onstack support
If we reinitialize a kthread_work that has been activated,
this will cause delayed_work_list/work_list corruption.
enable this config,there is an chance to fixup these errors
or WARNING the wrong use of kthread_work
[30858.395766] list_del corruption. next->prev should be ffe388ebbf88, but
was ffe388ebb588
[30858.395788] WARNING: CPU: 2 PID: 387 at kernel/msm-4.19/lib/list_debug.c:56
__list_del_entry_valid+0xc8/0xd0
...
[30858.395906] Call trace:
[30858.395909] __list_del_entry_valid+0xc8/0xd0
[30858.395912] __kthread_cancel_work_sync+0x98/0x138
[30858.395915] kthread_cancel_delayed_work_sync+0x10/0x20
[30858.395917] sde_encoder_resource_control+0xe8/0x12c0
[30858.395920] sde_encoder_prepare_for_kickoff+0x5dc/0x2008
[30858.395923] sde_crtc_commit_kickoff+0x280/0x890
[30858.395925] sde_kms_commit+0x16c/0x278
[30858.395928] complete_commit+0x3c4/0x760
[30858.395931] _msm_drm_commit_work_cb+0xec/0x1e0
[30858.395933] kthread_worker_fn+0xf8/0x190
[30858.395935] kthread+0x118/0x128
[30858.395938] ret_from_fork+0x10/0x18
crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
[ffe389392620] delayed_work_list = {
next = 0xffe388ebbf88,
prev = 0xffe388ebb588
}
crash> list 0xffe388ebbf88
ffe388ebbf88
Signed-off-by: Qianli Zhao
---
V5:
- Fix format error checked by checkpatch.pl
V4:
- Changelog update
- Add comment for KWORK_ENTRY_STATIC
- Code format modification
- Check worker availability early in kthread_flush_work
V3:
- changelog update
- add fixup_assert_init support
- move debug_kwork_activate/debug_kwork_deactivate before list operation
- name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
- use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
on stack
- __init_kwork before clear kthread_work in kthread_init_work
---
include/linux/kthread.h | 30 ++-
include/linux/poison.h | 4 ++
kernel/kthread.c| 136 ++--
lib/Kconfig.debug | 10
4 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..706302b 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,17 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +126,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -159,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
+ __init_kwork(work, 0);
\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ __init_kwork(work, 1);
\
memset((work), 0, sizeof(struct kthread_work)); \
INIT_LIST_HEAD(&(work)->node); \
(work)->func = (fn);\
@@ -172,6 +192,14 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
TIMER_IRQSAFE);\
} while (0)
+#define kthread_init_delayed_work_onstack(dwork, fn)
[PATCH v4] kthread: Add debugobject support
From: Qianli Zhao
Add debugobject support to track the life time of kthread_work
which is used to detect reinitialization/free active object problems
Add kthread_init_work_onstack()/kthread_init_delayed_work_onstack() for
kthread onstack support
If we reinitialize a kthread_work that has been activated,
this will cause delayed_work_list/work_list corruption.
enable this config,there is an chance to fixup these errors
or WARNING the wrong use of kthread_work
[30858.395766] list_del corruption. next->prev should be ffe388ebbf88, but
was ffe388ebb588
[30858.395788] WARNING: CPU: 2 PID: 387 at
/home/work/data/codes/build_home/kernel/msm-4.19/lib/list_debug.c:56
__list_del_entry_valid+0xc8/0xd0
...
[30858.395906] Call trace:
[30858.395909] __list_del_entry_valid+0xc8/0xd0
[30858.395912] __kthread_cancel_work_sync+0x98/0x138
[30858.395915] kthread_cancel_delayed_work_sync+0x10/0x20
[30858.395917] sde_encoder_resource_control+0xe8/0x12c0
[30858.395920] sde_encoder_prepare_for_kickoff+0x5dc/0x2008
[30858.395923] sde_crtc_commit_kickoff+0x280/0x890
[30858.395925] sde_kms_commit+0x16c/0x278
[30858.395928] complete_commit+0x3c4/0x760
[30858.395931] _msm_drm_commit_work_cb+0xec/0x1e0
[30858.395933] kthread_worker_fn+0xf8/0x190
[30858.395935] kthread+0x118/0x128
[30858.395938] ret_from_fork+0x10/0x18
crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
[ffe389392620] delayed_work_list = {
next = 0xffe388ebbf88,
prev = 0xffe388ebb588
}
crash> list 0xffe388ebbf88
ffe388ebbf88
Signed-off-by: Qianli Zhao
---
V4:
- Changelog update
- Add comment for KWORK_ENTRY_STATIC
- Code format modification
- Check worker availability early in kthread_flush_work
V3:
- changelog update
- add fixup_assert_init support
- move debug_kwork_activate/debug_kwork_deactivate before list operation
- name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
- use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
on stack
- __init_kwork before clear kthread_work in kthread_init_work
---
include/linux/kthread.h | 30 ++-
include/linux/poison.h | 4 ++
kernel/kthread.c| 135 ++--
lib/Kconfig.debug | 10
4 files changed, 173 insertions(+), 6 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..706302b 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,17 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +126,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -159,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
+ __init_kwork(work, 0);
\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ __init_kwork(work, 1);
\
memset((work), 0, sizeof(struct kthread_work)); \
INIT_LIST_HEAD(&(work)->node); \
(work)->func = (fn);\
@@ -172,6 +192,14 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
TIMER_IRQSAFE);\
} while (0)
+#define kthread_init_delayed_work_onstack(dwork, fn)
Re: [PATCH v3] kthread: add objectdebug support
Hi,Stephen
Thanks for your suggestion, i will improve my patch.
Thanks.
On Sat, Aug 15, 2020 at 4:18 PM Stephen Boyd wrote:
>
> Quoting Qianli Zhao (2020-08-13 02:55:16)
> > From: Qianli Zhao
> >
> > Add debugobject support to track the life time of kthread_work
>
> Subject says 'objectdebug' but then this says debugobject. Use
> debugobject throughout please.
>
> > which is used to detect reinitialization/free active object problems
>
> which is used to detect reinitialization/free active object problems.
>
> > Add kthread_init_work_onstack/kthread_init_delayed_work_onstack for
> > kthread onstack support
>
> kthread onstack support.
>
> Also, mark functions with parenthesis please.
>
> >
> > If we reinitialize a kthread_work that has been activated,
> > this will cause delayed_work_list/work_list corruption.
> > enable this config,there is an chance to fixup these errors
>
> Capitalize enable.
>
> > or WARNING the wrong use of kthread_work
> >
> > [30858.395766] list_del corruption. next->prev should be ffe388ebbf88,
> > but was ffe388ebb588
> > [30858.395788] WARNING: CPU: 2 PID: 387 at
> > /home/work/data/codes/build_home/kernel/msm-4.19/lib/list_debug.c:56
> > __list_del_entry_valid+0xc8/0xd0
> > ...
> > [30858.405951] list_add corruption. next->prev should be prev
> > (ffe389392620), but was ffe388ebbf88. (next=ffe388ebbf88).
> > [30858.405977] WARNING: CPU: 0 PID: 7721 at
> > /home/work/data/codes/build_home/kernel/msm-4.19/lib/list_debug.c:25
> > __list_add_valid+0x7c/0xc8
> >
> > crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
> > [ffe389392620] delayed_work_list = {
> > next = 0xffe388ebbf88,
> > prev = 0xffe388ebb588
> > }
> > crash> list 0xffe388ebbf88
> > ffe388ebbf88
> >
> > Signed-off-by: Qianli Zhao
> [...]
> > diff --git a/include/linux/poison.h b/include/linux/poison.h
> > index df34330..2e6a370 100644
> > --- a/include/linux/poison.h
> > +++ b/include/linux/poison.h
> > @@ -86,4 +86,7 @@
> > /** security/ **/
> > #define KEY_DESTROY0xbd
> >
> > +/** kernel/kthread **/
> > +#define KWORK_ENTRY_STATIC ((void *) 0x600 + POISON_POINTER_DELTA)
>
> Can we get a comment above this like there is for TIMER_ENTRY_STATIC?
>
> > +
> > #endif
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 132f84a..68a16cc 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -67,6 +67,118 @@ enum KTHREAD_BITS {
> > KTHREAD_SHOULD_PARK,
> > };
> >
> > +#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
> > +static struct debug_obj_descr kwork_debug_descr;
> > +
> > +static void *kwork_debug_hint(void *addr)
> > +{
> > + return ((struct kthread_work *) addr)->func;
> > +}
> > +
> > +static bool kwork_is_static_object(void *addr)
> > +{
> > + struct kthread_work *kwork = addr;
> > +
> > + return (kwork->node.prev == NULL &&
> > + kwork->node.next == KWORK_ENTRY_STATIC);
> > +}
> > +
> > +static bool kwork_fixup_init(void *addr, enum debug_obj_state state)
> > +{
> > + struct kthread_work *kwork = addr;
> > +
> > + switch (state) {
> > + case ODEBUG_STATE_ACTIVE:
> > + kthread_cancel_work_sync(kwork);
> > + debug_object_init(kwork, _debug_descr);
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static bool kwork_fixup_free(void *addr, enum debug_obj_state state)
> > +{
> > + struct kthread_work *kwork = addr;
> > +
> > + switch (state) {
> > + case ODEBUG_STATE_ACTIVE:
> > + kthread_cancel_work_sync(kwork);
> > + debug_object_free(kwork, _debug_descr);
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static void stub_kthread_work(struct kthread_work *unuse)
> > +{
> > + WARN_ON(1);
> > +}
> > +
> > +static bool kwork_fixup_assert_init(void *addr, enum debug_obj_state state)
> > +{
> > + struct kthread_work *kwork = addr;
> > + switch (state) {
> > + case ODEBUG_STATE_NOTAVAILABLE:
> > + kthread_init_work(kwork, stub_kthread_work);
>
Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer
Thomas Gleixner 于2020年8月13日周四 下午6:46写道： > > Qianli Zhao writes: > > Please start the first word after the colon with an upper case letter. > > > do_init_timer can specify flags of timer_list, > > Please write do_init_timer() so it's entirely clear that this is about a > function. > > > but this function does not expect to specify the CPU or idx. > > or idx does not mean anything unless someone looks at the > code. Changelogs want to explain things so they can be understood > without staring at the code. will update changelog > > If user invoking do_init_timer and specify CPU, > > The result may not what we expected. > > Right. And which caller exactly hands in crappy flags? This change is more of a sanity check to avoid these wrong use > > E.g: > > do_init_timer invoked in core2,and specify flags 0x1 > > final result flags is 0x3.If the specified CPU number > > exceeds the actual number,more serious problems will happen > > More serious problems is not a really helpful technical explanation and > 0x3 does not make sense for a changelog reader either because it again > requires to look up the code. > > > timer->entry.pprev = NULL; > > timer->function = func; > > - timer->flags = flags | raw_smp_processor_id(); > > + timer->flags = (flags & ~TIMER_BASEMASK & ~TIMER_ARRAYMASK) | > > raw_smp_processor_id(); > > If the caller hands in invalid flags then silently fixing them up is > fundamentally wrong. So this wants to be: > >if (WARN_ON(flags & ~TIMER_INIT_FLAGS)) > flags &= TIMER_INIT_FLAGS; > > and TIMER_INIT_FLAGS wants to be exactly the set of flags which are > valid for being handed in by a caller, i.e.: > > TIMER_DEFFERABLE, TIMER_PINNED, TIMER_IRQSAFE This change is very good，thanks for teaching > Guess what happens when the caller hands in TIMER_MIGRATING? If TIMER_MIGRATING is set in timer_setup, lock_timer_base will fall into a dead loop > If we do sanity checking then we do it correct and not just silently > papering over the particular problem which you ran into. Thanks for teaching. I have updated patchset,please review. > Thanks, > > tglx
[PATCH v2] timer: Mask illegal set of flags in do_init_timer()
From: Qianli Zhao
do_init_timer() can specify flags of timer_list,
only TIMER_DEFFERABLE, TIMER_PINNED, TIMER_IRQSAFE are legal
do a sanity check, mask and warning illegal set of flags
Signed-off-by: Qianli Zhao
---
V2:
- update changelog
- mask and warning illegal set
---
include/linux/timer.h | 1 +
kernel/time/timer.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/include/linux/timer.h b/include/linux/timer.h
index 07910ae..d10bc7e 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -67,6 +67,7 @@ struct timer_list {
#define TIMER_DEFERRABLE 0x0008
#define TIMER_PINNED 0x0010
#define TIMER_IRQSAFE 0x0020
+#define TIMER_INIT_FLAGS (TIMER_DEFERRABLE | TIMER_PINNED |
TIMER_IRQSAFE)
#define TIMER_ARRAYSHIFT 22
#define TIMER_ARRAYMASK0xFFC0
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 026ac01..f7398ab 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -789,6 +789,8 @@ static void do_init_timer(struct timer_list *timer,
{
timer->entry.pprev = NULL;
timer->function = func;
+ if (WARN_ON(flags & ~TIMER_INIT_FLAGS))
+ flags &= TIMER_INIT_FLAGS;
timer->flags = flags | raw_smp_processor_id();
lockdep_init_map(>lockdep_map, name, key, 0);
}
--
2.7.4
[PATCH v3] kthread: add objectdebug support
From: Qianli Zhao
Add debugobject support to track the life time of kthread_work
which is used to detect reinitialization/free active object problems
Add kthread_init_work_onstack/kthread_init_delayed_work_onstack for
kthread onstack support
If we reinitialize a kthread_work that has been activated,
this will cause delayed_work_list/work_list corruption.
enable this config,there is an chance to fixup these errors
or WARNING the wrong use of kthread_work
[30858.395766] list_del corruption. next->prev should be ffe388ebbf88, but
was ffe388ebb588
[30858.395788] WARNING: CPU: 2 PID: 387 at
/home/work/data/codes/build_home/kernel/msm-4.19/lib/list_debug.c:56
__list_del_entry_valid+0xc8/0xd0
...
[30858.405951] list_add corruption. next->prev should be prev
(ffe389392620), but was ffe388ebbf88. (next=ffe388ebbf88).
[30858.405977] WARNING: CPU: 0 PID: 7721 at
/home/work/data/codes/build_home/kernel/msm-4.19/lib/list_debug.c:25
__list_add_valid+0x7c/0xc8
crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
[ffe389392620] delayed_work_list = {
next = 0xffe388ebbf88,
prev = 0xffe388ebb588
}
crash> list 0xffe388ebbf88
ffe388ebbf88
Signed-off-by: Qianli Zhao
---
Thanks for your suggestions,tglx,Stephen
please review this patchset
changes:
V3:
- changelog update
- add fixup_assert_init support
- move debug_kwork_activate/debug_kwork_deactivate before list operation
- name the kconfig CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
- use kthread_init_work_onstack/destroy_kwork_on_stack when kthread_work used
on stack
- __init_kwork before clear kthread_work in kthread_init_work
---
include/linux/kthread.h | 29 +-
include/linux/poison.h | 3 +
kernel/kthread.c| 142 +---
lib/Kconfig.debug | 10
4 files changed, 176 insertions(+), 8 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..d3481a3 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,16 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD_WORK
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +125,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -159,6 +169,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
+ __init_kwork(work, 0);
\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ __init_kwork(work, 1);
\
memset((work), 0, sizeof(struct kthread_work)); \
INIT_LIST_HEAD(&(work)->node); \
(work)->func = (fn);\
@@ -172,6 +191,14 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
TIMER_IRQSAFE);\
} while (0)
+#define kthread_init_delayed_work_onstack(dwork, fn)
\
+ do {\
+ kthread_init_work_onstack(&(dwork)->work, (fn));
\
+ __init_timer_on_stack(&(dwork)->timer,
\
+kthread_delayed_work_timer_fn, \
+TIMER_IRQSAFE);\
+
[RFC V2] kthread: add object debug support
From: Qianli Zhao
Add debugobject support to track the life time of kthread_work
which is used to detect reinitialization/free active object problems
Add kthread_init_work_onstack/kthread_init_delayed_work_onstack for
kthread onstack support
Signed-off-by: Qianli Zhao
---
I got an crash issue since kthread_delayed_work reinitialization,crash log show
us that is a timer NULL pointer exception
[16238.089921] Unable to handle kernel paging request at virtual address
dead0208
[16238.090298] Call trace:
[16238.090307] run_timer_softirq+0x2d0/0xa30
[16238.090318] __do_softirq+0x1dc/0x384
[16238.090328] irq_exit+0xb4/0xb8
[16238.090338] __handle_domain_irq+0x84/0xc0
[16238.090345] gic_handle_irq+0x138/0x1bc
[16238.090353] el1_irq+0xb0/0x128
[16238.090362] queue_work_on+0x54/0x60
[16238.090374] _process_event_group+0x190/0x230
[16238.090382] kgsl_process_event_groups+0x44/0x70
[16238.090391] adreno_dispatcher_work+0x1e4/0xae8
[16238.090400] kthread_worker_fn+0xec/0x180
[16238.090407] kthread+0x118/0x128
[16238.090415] ret_from_fork+0x10/0x18
It turns out that the problem is caused by the timer reinitialization,after
enable CONFIG_DEBUG_OBJECTS_TIMERS,the reason for the problem is obvious.
This timer belongs to kthread_delayed_work,from kernel log we also see list
corruption WARNING:
[30858.395766] list_del corruption. next->prev should be ffe388ebbf88, but
was ffe388ebb588
[30858.395788] WARNING: CPU: 2 PID: 387 at
/home/work/data/miui_codes/build_home/kernel/msm-4.19/lib/list_debug.c:56
__list_del_entry_valid+0xc8/0xd0
...
[30858.405951] list_add corruption. next->prev should be prev
(ffe389392620), but was ffe388ebbf88. (next=ffe388ebbf88).
[30858.405977] WARNING: CPU: 0 PID: 7721 at
/home/work/data/miui_codes/build_home/kernel/msm-4.19/lib/list_debug.c:25
__list_add_valid+0x7c/0xc8
crash> struct kthread_worker.delayed_work_list 0xffe3893925f0
[ffe389392620] delayed_work_list = {
next = 0xffe388ebbf88,
prev = 0xffe388ebb588
}
crash> list 0xffe388ebbf88
ffe388ebbf88
delayed_work_list is corruption,this work is reinitialized.if kthread_work
reinitialized after move to work_list,this work will be carried out forever in
kthread_worker_fn.
Timer and workqueue both provide debugobject support，kthread_work similar with
workqueue,so i think it is necessary to support objectdebug in kthread_work.
---
include/linux/kthread.h | 29 ++-
include/linux/poison.h | 3 ++
kernel/kthread.c| 95 -
lib/Kconfig.debug | 10 ++
4 files changed, 135 insertions(+), 2 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..1530953 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,16 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +125,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -160,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
memset((work), 0, sizeof(struct kthread_work)); \
+ __init_kwork(work, 0);
\
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ __init_kwork(work, 1);
\
INIT_LIST_HEAD(&(work)->node);
[RFC] kthread: add object debug support
From: Qianli Zhao
Add debugobject support to track the life time of kthread_work
which is used to detect reinitialization/free active object problems
Add kthread_init_work_onstack/kthread_init_delayed_work_onstack for
kthread onstack support
Signed-off-by: Qianli Zhao
---
include/linux/kthread.h | 29 ++-
include/linux/poison.h | 3 ++
kernel/kthread.c| 95 -
lib/Kconfig.debug | 10 ++
4 files changed, 135 insertions(+), 2 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 65b81e0..1530953 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -108,6 +108,16 @@ struct kthread_delayed_work {
struct timer_list timer;
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD
+extern void __init_kwork(struct kthread_work *kwork, int onstack);
+extern void destroy_kwork_on_stack(struct kthread_work *kwork);
+extern void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork);
+#else
+static inline void __init_kwork(struct kthread_work *kwork, int onstack) { }
+static inline void destroy_kwork_on_stack(struct kthread_work *kwork) { }
+static inline void destroy_delayed_kwork_on_stack(struct kthread_delayed_work
*kdwork) { }
+#endif
+
#define KTHREAD_WORKER_INIT(worker){ \
.lock = __RAW_SPIN_LOCK_UNLOCKED((worker).lock),\
.work_list = LIST_HEAD_INIT((worker).work_list),\
@@ -115,7 +125,7 @@ struct kthread_delayed_work {
}
#define KTHREAD_WORK_INIT(work, fn){ \
- .node = LIST_HEAD_INIT((work).node),\
+ .node = { .next = KWORK_ENTRY_STATIC }, \
.func = (fn), \
}
@@ -160,6 +170,15 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
#define kthread_init_work(work, fn)\
do {\
memset((work), 0, sizeof(struct kthread_work)); \
+ __init_kwork(work, 0);
\
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn);\
+ } while (0)
+
+#define kthread_init_work_onstack(work, fn)
\
+ do {\
+ memset((work), 0, sizeof(struct kthread_work)); \
+ __init_kwork(work, 1);
\
INIT_LIST_HEAD(&(work)->node); \
(work)->func = (fn);\
} while (0)
@@ -172,6 +191,14 @@ extern void __kthread_init_worker(struct kthread_worker
*worker,
TIMER_IRQSAFE);\
} while (0)
+#define kthread_init_delayed_work_onstack(dwork, fn)
\
+ do {\
+ kthread_init_work_onstack(&(dwork)->work, (fn));
\
+ __init_timer_on_stack(&(dwork)->timer,
\
+kthread_delayed_work_timer_fn, \
+TIMER_IRQSAFE);\
+ } while (0)
+
int kthread_worker_fn(void *worker_ptr);
__printf(2, 3)
diff --git a/include/linux/poison.h b/include/linux/poison.h
index df34330..2e6a370 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -86,4 +86,7 @@
/** security/ **/
#define KEY_DESTROY0xbd
+/** kernel/kthread **/
+#define KWORK_ENTRY_STATIC ((void *) 0x600 + POISON_POINTER_DELTA)
+
#endif
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 132f84a..ca00bd2 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -67,6 +67,94 @@ enum KTHREAD_BITS {
KTHREAD_SHOULD_PARK,
};
+#ifdef CONFIG_DEBUG_OBJECTS_KTHREAD
+static struct debug_obj_descr kwork_debug_descr;
+
+static void *kwork_debug_hint(void *addr)
+{
+ return ((struct kthread_work *) addr)->func;
+}
+
+static bool kwork_is_static_object(void *addr)
+{
+ struct kthread_work *kwork = addr;
+
+ return (kwork->node.prev == NULL &&
+ kwork->node.next == KWORK_ENTRY_STATIC);
+}
+
+static bool kwork_fixup_init(void *addr, enum debug_obj_state state)
+{
+ struct kthread_work *kwork = addr;
+
+ switch (state) {
+ case ODEBUG_STATE_ACTIVE:
+ kthread_cancel_work_sync(kwork);
+ debug_object_init(kwork, _debug_descr);
+ return true;
+ default:
+
[PATCH] timer: mask unnecessary set of flags in do_init_timer
From: Qianli Zhao
do_init_timer can specify flags of timer_list,
but this function does not expect to specify the CPU or idx.
If user invoking do_init_timer and specify CPU,
The result may not what we expected.
E.g:
do_init_timer invoked in core2,and specify flags 0x1
final result flags is 0x3.If the specified CPU number
exceeds the actual number,more serious problems will happen
Signed-off-by: Qianli Zhao
---
kernel/time/timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 026ac01..17e3737 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -789,7 +789,7 @@ static void do_init_timer(struct timer_list *timer,
{
timer->entry.pprev = NULL;
timer->function = func;
- timer->flags = flags | raw_smp_processor_id();
+ timer->flags = (flags & ~TIMER_BASEMASK & ~TIMER_ARRAYMASK) |
raw_smp_processor_id();
lockdep_init_map(>lockdep_map, name, key, 0);
}
--
2.7.4
[PATCH] firewire: firewire-cdev.h: Avoid the use of one-element array
From: Qianli Zhao
There is a regular need in the kernel to provide a way to declare having a
dynamically sized set of trailing elements in a structure. Kernel code should
always use “flexible array members”[1] for these cases. The older style of
one-element or zero-length arrays should no longer be used[2].
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://github.com/KSPP/linux/issues/21
Signed-off-by: Qianli Zhao
---
include/uapi/linux/firewire-cdev.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/firewire-cdev.h
b/include/uapi/linux/firewire-cdev.h
index 7e5b5c1..487de87f 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -118,7 +118,7 @@ struct fw_cdev_event_response {
__u32 type;
__u32 rcode;
__u32 length;
- __u32 data[0];
+ __u32 data[];
};
/**
@@ -142,7 +142,7 @@ struct fw_cdev_event_request {
__u64 offset;
__u32 handle;
__u32 length;
- __u32 data[0];
+ __u32 data[];
};
/**
@@ -205,7 +205,7 @@ struct fw_cdev_event_request2 {
__u32 generation;
__u32 handle;
__u32 length;
- __u32 data[0];
+ __u32 data[];
};
/**
@@ -344,7 +344,7 @@ struct fw_cdev_event_iso_resource {
* @data: Incoming data
*
* If @type is %FW_CDEV_EVENT_PHY_PACKET_SENT, @length is 0 and @data empty,
- * except in case of a ping packet: Then, @length is 4, and @data[0] is the
+ * except in case of a ping packet: Then, @length is 4, and @data[] is the
* ping time in 49.152MHz clocks if @rcode is %RCODE_COMPLETE.
*
* If @type is %FW_CDEV_EVENT_PHY_PACKET_RECEIVED, @length is 8 and @data
@@ -355,7 +355,7 @@ struct fw_cdev_event_phy_packet {
__u32 type;
__u32 rcode;
__u32 length;
- __u32 data[0];
+ __u32 data[];
};
/**
--
2.7.4
[PATCH] mm: slab: Avoid the use of one-element array and use struct_size() helper
From: Qianli Zhao
There is a regular need in the kernel to provide a way to declare having a
dynamically sized set of trailing elements in a structure. Kernel code should
always use “flexible array members”[1] for these cases. The older style of
one-element or zero-length arrays should no longer be used[2].
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://github.com/KSPP/linux/issues/21
Signed-off-by: Qianli Zhao
---
mm/slab.h| 2 +-
mm/slab_common.c | 7 ++-
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/mm/slab.h b/mm/slab.h
index 74f7e09..c12fb65 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -34,7 +34,7 @@ struct kmem_cache {
struct memcg_cache_array {
struct rcu_head rcu;
- struct kmem_cache *entries[0];
+ struct kmem_cache *entries[];
};
/*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index fe8b684..56f4818 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -166,9 +166,7 @@ static int init_memcg_params(struct kmem_cache *s,
if (!memcg_nr_cache_ids)
return 0;
- arr = kvzalloc(sizeof(struct memcg_cache_array) +
- memcg_nr_cache_ids * sizeof(void *),
- GFP_KERNEL);
+ arr = kvzalloc(struct_size(arr, entries, memcg_nr_cache_ids),
GFP_KERNEL);
if (!arr)
return -ENOMEM;
@@ -199,8 +197,7 @@ static int update_memcg_params(struct kmem_cache *s, int
new_array_size)
{
struct memcg_cache_array *old, *new;
- new = kvzalloc(sizeof(struct memcg_cache_array) +
- new_array_size * sizeof(void *), GFP_KERNEL);
+ new = kvzalloc(struct_size(new, entries, new_array_size), GFP_KERNEL);
if (!new)
return -ENOMEM;
--
2.7.4
