Messages in this thread | | | Subject | Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die() | From | Tiezhu Yang <> | Date | Sat, 19 Aug 2023 10:44:01 +0800 |
| |
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
| |