Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()

[Tiezhu Yang]

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@xxxxxxxxxxxxxxxxx/
>
>  I think you meant:
>
> Link: https://lore.kernel.org/r/43DDF02E.76F0.0078.0@xxxxxxxxxx/
>
> 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