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 <[email protected]>
---
arch/mips/include/asm/ptrace.h | 2 +-
arch/mips/kernel/traps.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
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];
--
2.1.0
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/[email protected]/> 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/[email protected]/> 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