| Message ID | 1692005246-18399-3-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modify die() for MIPS | expand |
On Mon, 14 Aug 2023, Tiezhu Yang wrote: > If notify_die() returns NOTIFY_STOP, honor the return value from the > handler chain invocation in die() as, through a debugger, the fault > may have been fixed. It makes sense even if ignoring the event will > make the system unstable, by allowing access through a debugger it > has been compromised already anyway. So we can remove the noreturn > attribute for die() to make our port consistent with x86, arm64, > riscv and csky. I find it weird that you say that it is specifically the removal of the `noreturn' attribute that makes our port consistent with the other ones (and make it the change heading too). I don't think you need to mention the removal of `noreturn' even as you can see it in the code itself and it's a natural consequence of the change proper. How about: " MIPS: Do not kill the task in die() if notify_die() returns NOTIFY_STOP If notify_die() returns NOTIFY_STOP, honor the return value from the handler chain invocation in die() and return without killing the task as, through a debugger, the fault may have been fixed. It makes sense even if ignoring the event will make the system unstable: by allowing access through a debugger it has been compromised already anyway. It makes our port consistent with x86, arm64, riscv and csky. " then (notice the use of a colon rather than a comma changing the meaning of the sentence above)? > Commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die > chain handlers") may be the earliest of similar changes. > > Link: https://lore.kernel.org/all/alpine.DEB.2.21.2308132148500.8596@angie.orcam.me.uk/ I think you meant: Link: https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/ didn't you? > diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c > index 7a34674..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); It doesn't appear to me we should panic or execute the crash kernel if the oops is to be suppressed. Can we just do what the x86 port does, that is return if !sig after the call to `oops_exit'? Also I note that the individual ports aren't exactly consistent here with respect to each other, so maybe that's something you might want to post a combined follow-up clean-up patch series for too? Maciej
On 08/18/2023 10:41 AM, Maciej W. Rozycki wrote: > On Mon, 14 Aug 2023, Tiezhu Yang wrote: > >> If notify_die() returns NOTIFY_STOP, honor the return value from the >> handler chain invocation in die() as, through a debugger, the fault >> may have been fixed. It makes sense even if ignoring the event will >> make the system unstable, by allowing access through a debugger it >> has been compromised already anyway. So we can remove the noreturn >> attribute for die() to make our port consistent with x86, arm64, >> riscv and csky. > > I find it weird that you say that it is specifically the removal of the > `noreturn' attribute that makes our port consistent with the other ones > (and make it the change heading too). I don't think you need to mention > the removal of `noreturn' even as you can see it in the code itself and > it's a natural consequence of the change proper. How about: > > " > MIPS: Do not kill the task in die() if notify_die() returns NOTIFY_STOP > > If notify_die() returns NOTIFY_STOP, honor the return value from the > handler chain invocation in die() and return without killing the task > as, through a debugger, the fault may have been fixed. It makes sense > even if ignoring the event will make the system unstable: by allowing > access through a debugger it has been compromised already anyway. It > makes our port consistent with x86, arm64, riscv and csky. > " > > then (notice the use of a colon rather than a comma changing the meaning > of the sentence above)? OK, it looks better. > >> Commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die >> chain handlers") may be the earliest of similar changes. >> >> Link: https://lore.kernel.org/all/alpine.DEB.2.21.2308132148500.8596@angie.orcam.me.uk/ > > I think you meant: > > Link: https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/ > > didn't you? Yes, I will update it in v4. > >> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c >> index 7a34674..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); > > It doesn't appear to me we should panic or execute the crash kernel if > the oops is to be suppressed. Can we just do what the x86 port does, that > is return if !sig after the call to `oops_exit'? Yes, I think so, I will add a separate patch to do this. > > Also I note that the individual ports aren't exactly consistent here with > respect to each other, so maybe that's something you might want to post a > combined follow-up clean-up patch series for too? > Maybe do it someday if possible. Thanks, Tiezhu
diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h index daf3cf2..aee8e0a 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; +extern void die(const char *, struct pt_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 7a34674..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[];
If notify_die() returns NOTIFY_STOP, honor the return value from the handler chain invocation in die() as, through a debugger, the fault may have been fixed. It makes sense even if ignoring the event will make the system unstable, by allowing access through a debugger it has been compromised already anyway. So we can remove the noreturn attribute for die() to make our port consistent with x86, arm64, riscv and csky. Commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die chain handlers") may be the earliest of similar changes. Link: https://lore.kernel.org/all/alpine.DEB.2.21.2308132148500.8596@angie.orcam.me.uk/ Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- arch/mips/include/asm/ptrace.h | 2 +- arch/mips/kernel/traps.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-)