riscv: Dump user opcode bytes on fatal faults

[PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by Yunhui Cui 2 years, 10 months ago

We encountered such a problem that when the system starts to execute
init, init exits unexpectedly with error message: "unhandled signal 4
code 0x1 ...".

We are more curious about which instruction execution caused the
exception. After dumping it through show_opcodes(), we found that it
was caused by a floating-point instruction.

In this way, we found the problem: in the system bringup , it is
precisely that we have not enabled the floating point function(CONFIG_FPU
is set, but not enalbe COMPAT_HWCAP_ISA_F/D in the dts or acpi).

Like commit ba54d856a9d8 ("x86/fault: Dump user opcode bytes on fatal
faults"), when an exception occurs, it is necessary to dump the
instruction that caused the exception.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/include/asm/bug.h |  1 +
 arch/riscv/kernel/process.c  | 30 ++++++++++++++++++++++++++++++
 arch/riscv/kernel/traps.c    |  1 +
 3 files changed, 32 insertions(+)

diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h
index 1aaea81fb141..56dab998d05d 100644
--- a/arch/riscv/include/asm/bug.h
+++ b/arch/riscv/include/asm/bug.h
@@ -86,6 +86,7 @@ struct pt_regs;
 struct task_struct;
 
 void __show_regs(struct pt_regs *regs);
+void show_opcodes(struct pt_regs *regs);
 void die(struct pt_regs *regs, const char *str);
 void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr);
 
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 774ffde386ab..9ba9f8719605 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -83,6 +83,36 @@ void show_regs(struct pt_regs *regs)
 		dump_backtrace(regs, NULL, KERN_DEFAULT);
 }
 
+static int copy_code(struct pt_regs *regs, u8 *buf, unsigned long src,
+		     unsigned int nbytes)
+{
+	if (!user_mode(regs))
+		return copy_from_kernel_nofault(buf, (u8 *)src, nbytes);
+
+	/* The user space code from other tasks cannot be accessed. */
+	if (regs != task_pt_regs(current))
+		return -EPERM;
+
+	return copy_from_user_nofault(buf, (void __user *)src, nbytes);
+}
+
+void show_opcodes(struct pt_regs *regs)
+{
+	u8 opcodes[4];
+
+	switch (copy_code(regs, opcodes, regs->epc, sizeof(opcodes))) {
+	case 0:
+		pr_info("Opcode: %4ph", opcodes);
+		break;
+	case -EPERM:
+		pr_err("Unable to access userspace of other tasks");
+		break;
+	default:
+		pr_err("Failed to access opcode");
+		break;
+	}
+}
+
 #ifdef CONFIG_COMPAT
 static bool compat_mode_supported __read_mostly;
 
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index f6fda94e8e59..892826234ee9 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -100,6 +100,7 @@ void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr)
 		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
 		pr_cont("\n");
 		__show_regs(regs);
+		show_opcodes(regs);
 	}
 
 	force_sig_fault(signo, code, (void __user *)addr);
-- 
2.20.1

Re: [PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by Björn Töpel 2 years, 5 months ago

Hi Yunhui,

Waking up the dead! ;-)

Yunhui Cui <cuiyunhui@bytedance.com> writes:

> We encountered such a problem that when the system starts to execute
> init, init exits unexpectedly with error message: "unhandled signal 4
> code 0x1 ...".
>
> We are more curious about which instruction execution caused the
> exception. After dumping it through show_opcodes(), we found that it
> was caused by a floating-point instruction.
>
> In this way, we found the problem: in the system bringup , it is
> precisely that we have not enabled the floating point function(CONFIG_FPU
> is set, but not enalbe COMPAT_HWCAP_ISA_F/D in the dts or acpi).
>
> Like commit ba54d856a9d8 ("x86/fault: Dump user opcode bytes on fatal
> faults"), when an exception occurs, it is necessary to dump the
> instruction that caused the exception.

X86's show_opcodes() is used both for kernel oops:es, and userland
unhandled signals. On RISC-V there's dump_kernel_instr() added in commit
eb165bfa8eaf ("riscv: Add instruction dump to RISC-V splats").

Wdyt about reworking that function, so that it works for userland epc as
well? I think it's useful to have the surrounding instruction context,
and not just on instruction.

Björn

Re: [External] Re: [PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by yunhui cui 2 years, 5 months ago

Hi Björn,

On Wed, Aug 16, 2023 at 11:11 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> Hi Yunhui,
>
> Waking up the dead! ;-)
>

>
> X86's show_opcodes() is used both for kernel oops:es, and userland
> unhandled signals. On RISC-V there's dump_kernel_instr() added in commit
> eb165bfa8eaf ("riscv: Add instruction dump to RISC-V splats").
>
> Wdyt about reworking that function, so that it works for userland epc as
> well? I think it's useful to have the surrounding instruction context,
> and not just on instruction.

Okay,  Based on your suggestion, I'm going to rename dump_kernel_instr
to dump_instr. Like:
static void dump_instr(const char *loglvl, struct pt_regs *regs)
{
...
                if (user_mode(regs))
                        bad = get_user_nofault(val, &insns[i]);
                else
                        bad = get_kernel_nofault(val, &insns[i]);
...
}

What do you think?

Thanks,
Yunhui

Re: [External] Re: [PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by Björn Töpel 2 years, 5 months ago

yunhui cui <cuiyunhui@bytedance.com> writes:

> Hi Björn,
>
> On Wed, Aug 16, 2023 at 11:11 PM Björn Töpel <bjorn@kernel.org> wrote:
>>
>> Hi Yunhui,
>>
>> Waking up the dead! ;-)
>>
>
>>
>> X86's show_opcodes() is used both for kernel oops:es, and userland
>> unhandled signals. On RISC-V there's dump_kernel_instr() added in commit
>> eb165bfa8eaf ("riscv: Add instruction dump to RISC-V splats").
>>
>> Wdyt about reworking that function, so that it works for userland epc as
>> well? I think it's useful to have the surrounding instruction context,
>> and not just on instruction.
>
> Okay,  Based on your suggestion, I'm going to rename dump_kernel_instr
> to dump_instr. Like:
> static void dump_instr(const char *loglvl, struct pt_regs *regs)
> {
> ...
>                 if (user_mode(regs))
>                         bad = get_user_nofault(val, &insns[i]);
>                 else
>                         bad = get_kernel_nofault(val, &insns[i]);
> ...
> }
>
> What do you think?

Yeah, looks good! Does that work for you?


Björn

Re: [External] Re: [PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by yunhui cui 2 years, 5 months ago

Hi  Björn,

On Thu, Aug 17, 2023 at 6:03 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> yunhui cui <cuiyunhui@bytedance.com> writes:
>
> > Hi Björn,
> >
> > On Wed, Aug 16, 2023 at 11:11 PM Björn Töpel <bjorn@kernel.org> wrote:
> >>
> >> Hi Yunhui,
> >>
> >> Waking up the dead! ;-)
> >>
> >
> >>
> >> X86's show_opcodes() is used both for kernel oops:es, and userland
> >> unhandled signals. On RISC-V there's dump_kernel_instr() added in commit
> >> eb165bfa8eaf ("riscv: Add instruction dump to RISC-V splats").
> >>
> >> Wdyt about reworking that function, so that it works for userland epc as
> >> well? I think it's useful to have the surrounding instruction context,
> >> and not just on instruction.
> >
> > Okay,  Based on your suggestion, I'm going to rename dump_kernel_instr
> > to dump_instr. Like:
> > static void dump_instr(const char *loglvl, struct pt_regs *regs)
> > {
> > ...
> >                 if (user_mode(regs))
> >                         bad = get_user_nofault(val, &insns[i]);
> >                 else
> >                         bad = get_kernel_nofault(val, &insns[i]);
> > ...
> > }
> >
> > What do you think?
>
> Yeah, looks good! Does that work for you?
Yeah, It works for me, I'll update the patch to v3 and send it.

Thanks,
Yunhui

Re: [PATCH v2] riscv: Dump user opcode bytes on fatal faults

Posted by 运辉崔 2 years, 10 months ago

Dear All,

Any comments on this patch?

Thanks,
Yunhui