| Message ID | 20230829083614.117748-1-namcaov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b701f9e726f0a30a94ea6af596b74c1f07b95b6b |
| Headers | show |
| Series | [v2] riscv: provide riscv-specific is_trap_insn() | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD ef21fa7c198e |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 12 this patch: 12 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Thx for the fix. Reviewed-by: Guo Ren <guoren@kernel.org> On Tue, Aug 29, 2023 at 4:37 PM Nam Cao <namcaov@gmail.com> wrote: > > uprobes expects is_trap_insn() to return true for any trap instructions, > not just the one used for installing uprobe. The current default > implementation only returns true for 16-bit c.ebreak if C extension is > enabled. This can confuse uprobes if a 32-bit ebreak generates a trap > exception from userspace: uprobes asks is_trap_insn() who says there is no > trap, so uprobes assume a probe was there before but has been removed, and > return to the trap instruction. This causes an infinite loop of entering > and exiting trap handler. > > Instead of using the default implementation, implement this function > speficially for riscv with checks for both ebreak and c.ebreak. > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > Signed-off-by: Nam Cao <namcaov@gmail.com> > Tested-by: Björn Töpel <bjorn@rivosinc.com> > --- > v2: remove #ifdef CONFIG_RISCV_ISA_C (Guo Ren) > > arch/riscv/kernel/probes/uprobes.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > index 194f166b2cc4..4b3dc8beaf77 100644 > --- a/arch/riscv/kernel/probes/uprobes.c > +++ b/arch/riscv/kernel/probes/uprobes.c > @@ -3,6 +3,7 @@ > #include <linux/highmem.h> > #include <linux/ptrace.h> > #include <linux/uprobes.h> > +#include <asm/insn.h> > > #include "decode-insn.h" > > @@ -17,6 +18,11 @@ bool is_swbp_insn(uprobe_opcode_t *insn) > #endif > } > > +bool is_trap_insn(uprobe_opcode_t *insn) > +{ > + return riscv_insn_is_ebreak(*insn) || riscv_insn_is_c_ebreak(*insn); > +} > + > unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) > { > return instruction_pointer(regs); > -- > 2.34.1 >
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Tue, 29 Aug 2023 10:36:15 +0200 you wrote: > uprobes expects is_trap_insn() to return true for any trap instructions, > not just the one used for installing uprobe. The current default > implementation only returns true for 16-bit c.ebreak if C extension is > enabled. This can confuse uprobes if a 32-bit ebreak generates a trap > exception from userspace: uprobes asks is_trap_insn() who says there is no > trap, so uprobes assume a probe was there before but has been removed, and > return to the trap instruction. This causes an infinite loop of entering > and exiting trap handler. > > [...] Here is the summary with links: - [v2] riscv: provide riscv-specific is_trap_insn() https://git.kernel.org/riscv/c/b701f9e726f0 You are awesome, thank you!
diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c index 194f166b2cc4..4b3dc8beaf77 100644 --- a/arch/riscv/kernel/probes/uprobes.c +++ b/arch/riscv/kernel/probes/uprobes.c @@ -3,6 +3,7 @@ #include <linux/highmem.h> #include <linux/ptrace.h> #include <linux/uprobes.h> +#include <asm/insn.h> #include "decode-insn.h" @@ -17,6 +18,11 @@ bool is_swbp_insn(uprobe_opcode_t *insn) #endif } +bool is_trap_insn(uprobe_opcode_t *insn) +{ + return riscv_insn_is_ebreak(*insn) || riscv_insn_is_c_ebreak(*insn); +} + unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) { return instruction_pointer(regs);
uprobes expects is_trap_insn() to return true for any trap instructions, not just the one used for installing uprobe. The current default implementation only returns true for 16-bit c.ebreak if C extension is enabled. This can confuse uprobes if a 32-bit ebreak generates a trap exception from userspace: uprobes asks is_trap_insn() who says there is no trap, so uprobes assume a probe was there before but has been removed, and return to the trap instruction. This causes an infinite loop of entering and exiting trap handler. Instead of using the default implementation, implement this function speficially for riscv with checks for both ebreak and c.ebreak. Fixes: 74784081aac8 ("riscv: Add uprobes supported") Signed-off-by: Nam Cao <namcaov@gmail.com> Tested-by: Björn Töpel <bjorn@rivosinc.com> --- v2: remove #ifdef CONFIG_RISCV_ISA_C (Guo Ren) arch/riscv/kernel/probes/uprobes.c | 6 ++++++ 1 file changed, 6 insertions(+)