lkml.org 
[lkml]   [2023]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()
From
Date


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

\
 
 \ /
  Last update: 2023-08-19 04:45    [W:0.038 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site