| Message ID | 1690887599-11442-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Remove noreturn attribute for die() | expand |
On Tue, 1 Aug 2023, Tiezhu Yang wrote: > If notify_die() returns NOTIFY_STOP, there is no need to call > make_task_dead(), we can remove noreturn attribute for die(), > this is similar with arm64, riscv and csky. So you want to keep a task alive that has caused a kernel oops in the process context in this case, right? What purpose would it be for and what condition causes `notify_die' to return NOTIFY_STOP? IOW why is there no need to call `make_task_dead' in this case? Maciej
On 08/08/2023 10:54 PM, Maciej W. Rozycki wrote: > On Tue, 1 Aug 2023, Tiezhu Yang wrote: > >> If notify_die() returns NOTIFY_STOP, there is no need to call >> make_task_dead(), we can remove noreturn attribute for die(), >> this is similar with arm64, riscv and csky. > > So you want to keep a task alive that has caused a kernel oops in the > process context in this case, right? What purpose would it be for and > what condition causes `notify_die' to return NOTIFY_STOP? IOW why is > there no need to call `make_task_dead' in this case? > > Maciej > I did some research, hope it is useful. There is a related description in Documentation/input/notifier.rst: For each kind of event but the last, the callback may return NOTIFY_STOP in order to "eat" the event: the notify loop is stopped and the keyboard event is dropped. In commit 748f2edb5271 ("x86 NMI: better support for debuggers"), it said: If the notify is handled with a NOTIFY_STOP return, the system is given a new lease on life. In commit 004429956b48 ("handle recursive calls to bust_spinlocks()"), it said: However, at least on i386 die() has been capable of returning (and on other architectures this should really be that way, too) when notify_die() returns NOTIFY_STOP. In commit 22f5991c85de ("x86-64: honor notify_die() returning NOTIFY_STOP"), it said: This requires making die() return a value, making its callers honor this (and be prepared that it may return) In commit 620de2f5dc69 ("[IA64] honor notify_die() returning NOTIFY_STOP"), it said: This requires making die() and die_if_kernel() return a value, and their callers to honor this (and be prepared that it returns). Thanks, Tiezhu
On Wed, 9 Aug 2023, Tiezhu Yang wrote: > > So you want to keep a task alive that has caused a kernel oops in the > > process context in this case, right? What purpose would it be for and > > what condition causes `notify_die' to return NOTIFY_STOP? IOW why is > > there no need to call `make_task_dead' in this case? > > I did some research, hope it is useful. > > There is a related description in Documentation/input/notifier.rst: > > For each kind of event but the last, the callback may return > NOTIFY_STOP in order to "eat" the event: the notify loop is > stopped and the keyboard event is dropped. I saw that, but this is irrelevant. Dropping a keyboard event won't make the system unstable (though it can make a console user unstable, out of irritation). > In commit 748f2edb5271 ("x86 NMI: better support for debuggers"), it said: > > If the notify is handled with a NOTIFY_STOP return, the > system is given a new lease on life. > > In commit 004429956b48 ("handle recursive calls to bust_spinlocks()"), > it said: > > However, at least on i386 die() has been capable of returning > (and on other architectures this should really be that way, too) > when notify_die() returns NOTIFY_STOP. > > In commit 22f5991c85de ("x86-64: honor notify_die() returning NOTIFY_STOP"), > it said: > > This requires making die() return a value, making its callers honor > this (and be prepared that it may return) > > In commit 620de2f5dc69 ("[IA64] honor notify_die() returning NOTIFY_STOP"), > it said: > > This requires making die() and die_if_kernel() return a value, > and their callers to honor this (and be prepared that it returns). Thanks, that indeed helps, though indirectly. I think the most relevant, though still terse explanation comes from commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die chain handlers"), which I believe is the earliest of similar changes. The patch was originally submitted here: <https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/> and hardly any discussion emerged, but I think the key statement is: "[...] honor the return value from the handler chain invocation in die() as, through a debugger, the fault may have been fixed." Now it makes sense to me: even if ignoring the event will make the system unstable, by allowing access through a debugger it has been compromised already anyway. So I think your change will be good if you update the change description to include the justification quoted above rather than just: "the others do it too, so it must be good" (though you can of course mention that your change also makes our port consistent with other ones). I suggest linking to the original i386 submission too for future reference. Also I note that you combine three independent changes into one, so please split it into individual patches as per our requirements. Maciej
On 08/14/2023 05:30 AM, Maciej W. Rozycki wrote: > On Wed, 9 Aug 2023, Tiezhu Yang wrote: > >>> So you want to keep a task alive that has caused a kernel oops in the >>> process context in this case, right? What purpose would it be for and >>> what condition causes `notify_die' to return NOTIFY_STOP? IOW why is >>> there no need to call `make_task_dead' in this case? >> >> I did some research, hope it is useful. ... >> >> This requires making die() and die_if_kernel() return a value, >> and their callers to honor this (and be prepared that it returns). > > Thanks, that indeed helps, though indirectly. I think the most relevant, > though still terse explanation comes from commit 20c0d2d44029 ("[PATCH] > i386: pass proper trap numbers to die chain handlers"), which I believe is > the earliest of similar changes. The patch was originally submitted here: > <https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/> and hardly > any discussion emerged, but I think the key statement is: > > "[...] honor the return value from the handler chain invocation in die() > as, through a debugger, the fault may have been fixed." > > Now it makes sense to me: even if ignoring the event will make the system > unstable, by allowing access through a debugger it has been compromised > already anyway. > > So I think your change will be good if you update the change description > to include the justification quoted above rather than just: "the others do > it too, so it must be good" (though you can of course mention that your > change also makes our port consistent with other ones). I suggest linking > to the original i386 submission too for future reference. Thank you very much. > > Also I note that you combine three independent changes into one, so > please split it into individual patches as per our requirements. > Will do it in v2. Thanks, Tiezhu
diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h index daf3cf2..14d2ee9 100644 --- a/arch/mips/include/asm/ptrace.h +++ b/arch/mips/include/asm/ptrace.h @@ -159,7 +159,7 @@ static inline long regs_return_value(struct pt_regs *regs) extern asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall); extern asmlinkage void syscall_trace_leave(struct pt_regs *regs); -extern void die(const char *, struct pt_regs *) __noreturn; +void die(const char *str, struct pt_regs *regs); static inline void die_if_kernel(const char *str, struct pt_regs *regs) { diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index 246c6a6..4f5140f 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -391,16 +391,15 @@ void show_registers(struct pt_regs *regs) static DEFINE_RAW_SPINLOCK(die_lock); -void __noreturn die(const char *str, struct pt_regs *regs) +void die(const char *str, struct pt_regs *regs) { static int die_counter; - int sig = SIGSEGV; + int ret; oops_enter(); - if (notify_die(DIE_OOPS, str, regs, 0, current->thread.trap_nr, - SIGSEGV) == NOTIFY_STOP) - sig = 0; + ret = notify_die(DIE_OOPS, str, regs, 0, + current->thread.trap_nr, SIGSEGV); console_verbose(); raw_spin_lock_irq(&die_lock); @@ -422,7 +421,8 @@ void __noreturn die(const char *str, struct pt_regs *regs) if (regs && kexec_should_crash(current)) crash_kexec(regs); - make_task_dead(sig); + if (ret != NOTIFY_STOP) + make_task_dead(SIGSEGV); } extern struct exception_table_entry __start___dbe_table[]; @@ -1986,7 +1986,7 @@ int register_nmi_notifier(struct notifier_block *nb) return raw_notifier_chain_register(&nmi_chain, nb); } -void __noreturn nmi_exception_handler(struct pt_regs *regs) +void nmi_exception_handler(struct pt_regs *regs) { char str[100];
If notify_die() returns NOTIFY_STOP, there is no need to call make_task_dead(), we can remove noreturn attribute for die(), this is similar with arm64, riscv and csky. While at it, modify the die() declaration in ptrace.h to fix the following checkpatch warnings: WARNING: function definition argument 'const char *' should also have an identifier name WARNING: function definition argument 'struct pt_regs *' should also have an identifier name Additionally, also remove noreturn attribute for nmi_exception_handler due to it calls die(), otherwise there exists the following build error: arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror] Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- arch/mips/include/asm/ptrace.h | 2 +- arch/mips/kernel/traps.c | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-)