* [PATCH v3 0/3] Modify die() for MIPS
@ 2023-08-14 9:27 Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler() Tiezhu Yang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-14 9:27 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Maciej W. Rozycki, linux-mips, linux-kernel, loongson-kernel
This is based on 6.5-rc6.
There is a related build warning reported by kernel test robot:
warning: 'noreturn' function does return
it is better to fix it, so send this v3.
v3:
-- Make each patch can be built without errors and warnings.
v2:
-- Update the commit message to give more detailed info, split into
three individual patches, suggested by Maciej, thank you.
Tiezhu Yang (3):
MIPS: Remove noreturn attribute for nmi_exception_handler()
MIPS: Remove noreturn attribute for die()
MIPS: Modify the declaration for die()
arch/mips/include/asm/ptrace.h | 2 +-
arch/mips/kernel/traps.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler()
2023-08-14 9:27 [PATCH v3 0/3] Modify die() for MIPS Tiezhu Yang
@ 2023-08-14 9:27 ` Tiezhu Yang
2023-08-18 2:39 ` Maciej W. Rozycki
2023-08-14 9:27 ` [PATCH v3 2/3] MIPS: Remove noreturn attribute for die() Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 3/3] MIPS: Modify the declaration " Tiezhu Yang
2 siblings, 1 reply; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-14 9:27 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Maciej W. Rozycki, linux-mips, linux-kernel, loongson-kernel
In the later patch, we will remove noreturn attribute for die(), in order
to make each patch can be built without errors and warnings, just remove
noreturn attribute for nmi_exception_handler() earlier because it calls
die(), otherwise there exists the following build error after the later
patch:
arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror]
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/mips/kernel/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 246c6a6..7a34674 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()
2023-08-14 9:27 [PATCH v3 0/3] Modify die() for MIPS Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler() Tiezhu Yang
@ 2023-08-14 9:27 ` Tiezhu Yang
2023-08-18 2:41 ` Maciej W. Rozycki
2023-08-14 9:27 ` [PATCH v3 3/3] MIPS: Modify the declaration " Tiezhu Yang
2 siblings, 1 reply; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-14 9:27 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Maciej W. Rozycki, linux-mips, linux-kernel, loongson-kernel
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(-)
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[];
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] MIPS: Modify the declaration for die()
2023-08-14 9:27 [PATCH v3 0/3] Modify die() for MIPS Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler() Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 2/3] MIPS: Remove noreturn attribute for die() Tiezhu Yang
@ 2023-08-14 9:27 ` Tiezhu Yang
2023-08-18 2:42 ` Maciej W. Rozycki
2 siblings, 1 reply; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-14 9:27 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Maciej W. Rozycki, linux-mips, linux-kernel, loongson-kernel
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
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
arch/mips/include/asm/ptrace.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index aee8e0a..d05844e 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 *);
+extern void die(const char *str, struct pt_regs *regs);
static inline void die_if_kernel(const char *str, struct pt_regs *regs)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler()
2023-08-14 9:27 ` [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler() Tiezhu Yang
@ 2023-08-18 2:39 ` Maciej W. Rozycki
2023-08-19 2:38 ` Tiezhu Yang
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-18 2:39 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
On Mon, 14 Aug 2023, Tiezhu Yang wrote:
> In the later patch, we will remove noreturn attribute for die(), in order
> to make each patch can be built without errors and warnings, just remove
> noreturn attribute for nmi_exception_handler() earlier because it calls
> die(), otherwise there exists the following build error after the later
> patch:
I find the wording a bit odd here, but you'll have to rewrite the change
description for the update requested below, so let's defer any style fixes
to v4.
> arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror]
Now that I've looked into it in detail, this change is incomplete and
will make the kernel go astray if `nmi_exception_handler' actually ever
does return. See code in arch/mips/kernel/genex.S, which calls this
function and doesn't expect it to return. It has to be fixed before 2/3
can be considered. I wonder how you didn't catch it: you did check how
this code is used, didn't you?
Before submitting an updated version can you actually arrange for the
NOTIFY_STOP condition to happen in your lab and verify it is handled as
expected? And what was the motivation for this code update, just a
hypothetical scenario?
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()
2023-08-14 9:27 ` [PATCH v3 2/3] MIPS: Remove noreturn attribute for die() Tiezhu Yang
@ 2023-08-18 2:41 ` Maciej W. Rozycki
2023-08-19 2:44 ` Tiezhu Yang
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-18 2:41 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] MIPS: Modify the declaration for die()
2023-08-14 9:27 ` [PATCH v3 3/3] MIPS: Modify the declaration " Tiezhu Yang
@ 2023-08-18 2:42 ` Maciej W. Rozycki
2023-08-19 2:44 ` Tiezhu Yang
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-18 2:42 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
On Mon, 14 Aug 2023, Tiezhu Yang wrote:
> While at it, modify the die() declaration in ptrace.h to fix
> the following checkpatch warnings:
It doesn't make sense to say: "While at it" now that it's a separate
change, so how about:
"
Modify the die() declaration in ptrace.h to fix the following checkpatch
warnings:
"
?
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler()
2023-08-18 2:39 ` Maciej W. Rozycki
@ 2023-08-19 2:38 ` Tiezhu Yang
0 siblings, 0 replies; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-19 2:38 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
On 08/18/2023 10:39 AM, Maciej W. Rozycki wrote:
> On Mon, 14 Aug 2023, Tiezhu Yang wrote:
>
>> In the later patch, we will remove noreturn attribute for die(), in order
>> to make each patch can be built without errors and warnings, just remove
>> noreturn attribute for nmi_exception_handler() earlier because it calls
>> die(), otherwise there exists the following build error after the later
>> patch:
>
> I find the wording a bit odd here, but you'll have to rewrite the change
> description for the update requested below, so let's defer any style fixes
> to v4.
>
>> arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror]
>
> Now that I've looked into it in detail, this change is incomplete and
> will make the kernel go astray if `nmi_exception_handler' actually ever
> does return. See code in arch/mips/kernel/genex.S, which calls this
> function and doesn't expect it to return. It has to be fixed before 2/3
> can be considered. I wonder how you didn't catch it: you did check how
> this code is used, didn't you?
>
I think the proper way is to keep the noreturn attribute for
nmi_exception_handler(), and add a noreturn function BUG() at
the end of nmi_exception_handler() to make sure it does not
return.
> Before submitting an updated version can you actually arrange for the
> NOTIFY_STOP condition to happen in your lab and verify it is handled as
> expected? And what was the motivation for this code update, just a
> hypothetical scenario?
Yes, just a hypothetical scenario.
Thanks,
Tiezhu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] MIPS: Remove noreturn attribute for die()
2023-08-18 2:41 ` Maciej W. Rozycki
@ 2023-08-19 2:44 ` Tiezhu Yang
0 siblings, 0 replies; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-19 2:44 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] MIPS: Modify the declaration for die()
2023-08-18 2:42 ` Maciej W. Rozycki
@ 2023-08-19 2:44 ` Tiezhu Yang
0 siblings, 0 replies; 10+ messages in thread
From: Tiezhu Yang @ 2023-08-19 2:44 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Thomas Bogendoerfer, linux-mips, linux-kernel, loongson-kernel
On 08/18/2023 10:42 AM, Maciej W. Rozycki wrote:
> On Mon, 14 Aug 2023, Tiezhu Yang wrote:
>
>> While at it, modify the die() declaration in ptrace.h to fix
>> the following checkpatch warnings:
>
> It doesn't make sense to say: "While at it" now that it's a separate
> change, so how about:
>
> "
> Modify the die() declaration in ptrace.h to fix the following checkpatch
> warnings:
> "
OK, I will update it in v4.
Thanks,
Tiezhu
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-19 2:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-14 9:27 [PATCH v3 0/3] Modify die() for MIPS Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 1/3] MIPS: Remove noreturn attribute for nmi_exception_handler() Tiezhu Yang
2023-08-18 2:39 ` Maciej W. Rozycki
2023-08-19 2:38 ` Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 2/3] MIPS: Remove noreturn attribute for die() Tiezhu Yang
2023-08-18 2:41 ` Maciej W. Rozycki
2023-08-19 2:44 ` Tiezhu Yang
2023-08-14 9:27 ` [PATCH v3 3/3] MIPS: Modify the declaration " Tiezhu Yang
2023-08-18 2:42 ` Maciej W. Rozycki
2023-08-19 2:44 ` Tiezhu Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.