| Message ID | 20230827205641.46836-1-namcaov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | 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, 22 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 |
Nam Cao <namcaov@gmail.com> writes: > 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 cause an infinite loop of entering > and exiting trap handler. > > Instead of using the default implementation, implement this function > speficially for riscv which checks for both ebreak and c.ebreak. I took this for a spin, and it indeed fixes this new hang! Nice! However, when I tried setting an uprobe on the ebreak instruction (offset 0x118) from your example [1], the probe does not show up in the trace buffer. Any ideas? Regardless, your patch fixes the hang: Tested-by: Björn Töpel <bjorn@rivosinc.com> [1] https://lore.kernel.org/linux-riscv/ZOum50Py8Vki+Nd3@nam-dell/
On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: > Nam Cao <namcaov@gmail.com> writes: > > > 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 cause an infinite loop of entering > > and exiting trap handler. > > > > Instead of using the default implementation, implement this function > > speficially for riscv which checks for both ebreak and c.ebreak. > > I took this for a spin, and it indeed fixes this new hang! Nice! Great! Thanks for testing it. > However, when I tried setting an uprobe on the ebreak instruction > (offset 0x118) from your example [1], the probe does not show up in the > trace buffer. > > Any ideas? From my understanding, both uprobes and kprobes refuse to install break points into existing trap instructions. Otherwise, we may conflict with something else that is also using trap instructions. Best regards, Nam
On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: > On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: > > Nam Cao <namcaov@gmail.com> writes: > > > > > 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 cause an infinite loop of entering > > > and exiting trap handler. > > > > > > Instead of using the default implementation, implement this function > > > speficially for riscv which checks for both ebreak and c.ebreak. > > > > I took this for a spin, and it indeed fixes this new hang! Nice! > > Great! Thanks for testing it. > > > However, when I tried setting an uprobe on the ebreak instruction > > (offset 0x118) from your example [1], the probe does not show up in the > > trace buffer. > > > > Any ideas? > > >From my understanding, both uprobes and kprobes refuse to install break points > into existing trap instructions. Otherwise, we may conflict with something else > that is also using trap instructions. I just realize you probably ask this because uprobe can still be installed before applying the patch. But I think that is another bug that my patch also accidentally fix: uprobes should not install breakpoint into ebreak instructions, but it incorrectly does so because it does not even know about the existence of 32-bit ebreak. Best regards, Nam
On Mon, Aug 28, 2023 at 4:56 AM 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 cause an infinite loop of entering > and exiting trap handler. > > Instead of using the default implementation, implement this function > speficially for riscv which checks for both ebreak and c.ebreak. > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > Signed-off-by: Nam Cao <namcaov@gmail.com> > --- > arch/riscv/kernel/probes/uprobes.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > index 194f166b2cc4..91f4ce101cd1 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,15 @@ bool is_swbp_insn(uprobe_opcode_t *insn) > #endif > } > > +bool is_trap_insn(uprobe_opcode_t *insn) > +{ > +#ifdef CONFIG_RISCV_ISA_C Can we remove the CONFIG_RISCV_ISA_C? As you said, "uprobes expects is_trap_insn() to return true for any trap instructions". So userspace wouldn't be limited by CONFIG_RISCV_ISA_C. > + if (riscv_insn_is_c_ebreak(*insn)) > + return true; > +#endif > + return riscv_insn_is_ebreak(*insn); > +} > + > unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) > { > return instruction_pointer(regs); > -- > 2.34.1 >
Nam Cao <namcaov@gmail.com> writes: > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: >> > Nam Cao <namcaov@gmail.com> writes: >> > >> > > 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 cause an infinite loop of entering >> > > and exiting trap handler. >> > > >> > > Instead of using the default implementation, implement this function >> > > speficially for riscv which checks for both ebreak and c.ebreak. >> > >> > I took this for a spin, and it indeed fixes this new hang! Nice! >> >> Great! Thanks for testing it. >> >> > However, when I tried setting an uprobe on the ebreak instruction >> > (offset 0x118) from your example [1], the probe does not show up in the >> > trace buffer. >> > >> > Any ideas? >> >> >From my understanding, both uprobes and kprobes refuse to install break points >> into existing trap instructions. Otherwise, we may conflict with something else >> that is also using trap instructions. > > I just realize you probably ask this because uprobe can still be installed before > applying the patch. But I think that is another bug that my patch also > accidentally fix: uprobes should not install breakpoint into ebreak instructions, > but it incorrectly does so because it does not even know about the existence of > 32-bit ebreak. FWIW, I can still install the uprobe at an ebreak with you patch. It's not hit, but succeeds to install.
On Tue, Aug 29, 2023 at 01:56:34PM +0800, Guo Ren wrote: > On Mon, Aug 28, 2023 at 4:56 AM 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 cause an infinite loop of entering > > and exiting trap handler. > > > > Instead of using the default implementation, implement this function > > speficially for riscv which checks for both ebreak and c.ebreak. > > > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > > Signed-off-by: Nam Cao <namcaov@gmail.com> > > --- > > arch/riscv/kernel/probes/uprobes.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > > index 194f166b2cc4..91f4ce101cd1 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,15 @@ bool is_swbp_insn(uprobe_opcode_t *insn) > > #endif > > } > > > > +bool is_trap_insn(uprobe_opcode_t *insn) > > +{ > > +#ifdef CONFIG_RISCV_ISA_C > Can we remove the CONFIG_RISCV_ISA_C? As you said, "uprobes expects > is_trap_insn() to return true for any trap instructions". So userspace > wouldn't be limited by CONFIG_RISCV_ISA_C. Isn't the RISCV_ISA_C required because there's a different encoding for EBREAK vs C_EBREAK? That said, this should be using IS_ENABLED() not #ifdef, since the definition for riscv_insn_is_c_ebreak() is provided unconditionally afaict. > > > + if (riscv_insn_is_c_ebreak(*insn)) > > + return true; > > +#endif > > + return riscv_insn_is_ebreak(*insn); > > +} > > + > > unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) > > { > > return instruction_pointer(regs); > > -- > > 2.34.1 > >
On Tue, Aug 29, 2023 at 07:26:54AM +0100, Conor Dooley wrote: > On Tue, Aug 29, 2023 at 01:56:34PM +0800, Guo Ren wrote: > > On Mon, Aug 28, 2023 at 4:56 AM 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 cause an infinite loop of entering > > > and exiting trap handler. > > > > > > Instead of using the default implementation, implement this function > > > speficially for riscv which checks for both ebreak and c.ebreak. > > > > > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > > > Signed-off-by: Nam Cao <namcaov@gmail.com> > > > --- > > > arch/riscv/kernel/probes/uprobes.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > > > index 194f166b2cc4..91f4ce101cd1 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,15 @@ bool is_swbp_insn(uprobe_opcode_t *insn) > > > #endif > > > } > > > > > > +bool is_trap_insn(uprobe_opcode_t *insn) > > > +{ > > > +#ifdef CONFIG_RISCV_ISA_C > > > Can we remove the CONFIG_RISCV_ISA_C? As you said, "uprobes expects > > is_trap_insn() to return true for any trap instructions". So userspace > > wouldn't be limited by CONFIG_RISCV_ISA_C. > > Isn't the RISCV_ISA_C required because there's a different encoding for > EBREAK vs C_EBREAK? riscv_insn_is_c_ebreak() can be used without enabling RISCV_ISA_C, so no it's not required. Best regards, Nam
On Tue, Aug 29, 2023 at 07:26:54AM +0100, Conor Dooley wrote: > On Tue, Aug 29, 2023 at 01:56:34PM +0800, Guo Ren wrote: > > On Mon, Aug 28, 2023 at 4:56 AM 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 cause an infinite loop of entering > > > and exiting trap handler. > > > > > > Instead of using the default implementation, implement this function > > > speficially for riscv which checks for both ebreak and c.ebreak. > > > > > > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > > > Signed-off-by: Nam Cao <namcaov@gmail.com> > > > --- > > > arch/riscv/kernel/probes/uprobes.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c > > > index 194f166b2cc4..91f4ce101cd1 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,15 @@ bool is_swbp_insn(uprobe_opcode_t *insn) > > > #endif > > > } > > > > > > +bool is_trap_insn(uprobe_opcode_t *insn) > > > +{ > > > +#ifdef CONFIG_RISCV_ISA_C > > > Can we remove the CONFIG_RISCV_ISA_C? As you said, "uprobes expects > > is_trap_insn() to return true for any trap instructions". So userspace > > wouldn't be limited by CONFIG_RISCV_ISA_C. > > Isn't the RISCV_ISA_C required because there's a different encoding for > EBREAK vs C_EBREAK? That said, this should be using IS_ENABLED() not > #ifdef, since the definition for riscv_insn_is_c_ebreak() is provided > unconditionally afaict. Sorry, was too quick that I missed the last sentence. Now I'm not sure what you mean. But I agree with Guo Ren here, users can use compressed instructions but kernel does not have it enabled. So we should always check c.ebreak regardless of RISCV_ISA_C. Best regards, Nam
On Tue, Aug 29, 2023 at 08:14:59AM +0200, Björn Töpel wrote: > Nam Cao <namcaov@gmail.com> writes: > > > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: > >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: > >> > Nam Cao <namcaov@gmail.com> writes: > >> > > >> > > 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 cause an infinite loop of entering > >> > > and exiting trap handler. > >> > > > >> > > Instead of using the default implementation, implement this function > >> > > speficially for riscv which checks for both ebreak and c.ebreak. > >> > > >> > I took this for a spin, and it indeed fixes this new hang! Nice! > >> > >> Great! Thanks for testing it. > >> > >> > However, when I tried setting an uprobe on the ebreak instruction > >> > (offset 0x118) from your example [1], the probe does not show up in the > >> > trace buffer. > >> > > >> > Any ideas? > >> > >> >From my understanding, both uprobes and kprobes refuse to install break points > >> into existing trap instructions. Otherwise, we may conflict with something else > >> that is also using trap instructions. > > > > I just realize you probably ask this because uprobe can still be installed before > > applying the patch. But I think that is another bug that my patch also > > accidentally fix: uprobes should not install breakpoint into ebreak instructions, > > but it incorrectly does so because it does not even know about the existence of > > 32-bit ebreak. > > FWIW, I can still install the uprobe at an ebreak with you patch. It's > not hit, but succeeds to install. It seems uprobes install failures are completely silent (see uprobe_mmap() in kernel/events/uprobes.c). So I think although uprobes install seems fine, it actually is not. Best regards, Nam
Nam Cao <namcaov@gmail.com> writes: > On Tue, Aug 29, 2023 at 08:14:59AM +0200, Björn Töpel wrote: >> Nam Cao <namcaov@gmail.com> writes: >> >> > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: >> >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: >> >> > Nam Cao <namcaov@gmail.com> writes: >> >> > >> >> > > 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 cause an infinite loop of entering >> >> > > and exiting trap handler. >> >> > > >> >> > > Instead of using the default implementation, implement this function >> >> > > speficially for riscv which checks for both ebreak and c.ebreak. >> >> > >> >> > I took this for a spin, and it indeed fixes this new hang! Nice! >> >> >> >> Great! Thanks for testing it. >> >> >> >> > However, when I tried setting an uprobe on the ebreak instruction >> >> > (offset 0x118) from your example [1], the probe does not show up in the >> >> > trace buffer. >> >> > >> >> > Any ideas? >> >> >> >> >From my understanding, both uprobes and kprobes refuse to install break points >> >> into existing trap instructions. Otherwise, we may conflict with something else >> >> that is also using trap instructions. >> > >> > I just realize you probably ask this because uprobe can still be installed before >> > applying the patch. But I think that is another bug that my patch also >> > accidentally fix: uprobes should not install breakpoint into ebreak instructions, >> > but it incorrectly does so because it does not even know about the existence of >> > 32-bit ebreak. >> >> FWIW, I can still install the uprobe at an ebreak with you patch. It's >> not hit, but succeeds to install. > > It seems uprobes install failures are completely silent (see uprobe_mmap() in > kernel/events/uprobes.c). So I think although uprobes install seems fine, it > actually is not. Huh, so there's no check if the instruction is a valid one at event register point?
On Wed, Aug 30, 2023 at 9:32 AM Björn Töpel <bjorn@kernel.org> wrote: > > Nam Cao <namcaov@gmail.com> writes: > > > On Tue, Aug 29, 2023 at 08:14:59AM +0200, Björn Töpel wrote: > >> Nam Cao <namcaov@gmail.com> writes: > >> > >> > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: > >> >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: > >> >> > Nam Cao <namcaov@gmail.com> writes: > >> >> > > >> >> > > 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 cause an infinite loop of entering > >> >> > > and exiting trap handler. > >> >> > > > >> >> > > Instead of using the default implementation, implement this function > >> >> > > speficially for riscv which checks for both ebreak and c.ebreak. > >> >> > > >> >> > I took this for a spin, and it indeed fixes this new hang! Nice! > >> >> > >> >> Great! Thanks for testing it. > >> >> > >> >> > However, when I tried setting an uprobe on the ebreak instruction > >> >> > (offset 0x118) from your example [1], the probe does not show up in the > >> >> > trace buffer. > >> >> > > >> >> > Any ideas? > >> >> > >> >> >From my understanding, both uprobes and kprobes refuse to install break points > >> >> into existing trap instructions. Otherwise, we may conflict with something else > >> >> that is also using trap instructions. > >> > > >> > I just realize you probably ask this because uprobe can still be installed before > >> > applying the patch. But I think that is another bug that my patch also > >> > accidentally fix: uprobes should not install breakpoint into ebreak instructions, > >> > but it incorrectly does so because it does not even know about the existence of > >> > 32-bit ebreak. > >> > >> FWIW, I can still install the uprobe at an ebreak with you patch. It's > >> not hit, but succeeds to install. > > > > It seems uprobes install failures are completely silent (see uprobe_mmap() in > > kernel/events/uprobes.c). So I think although uprobes install seems fine, it > > actually is not. > > Huh, so there's no check if the instruction is a valid one at event > register point? There are some checks (eg. if the probe is within the binary), but they are not complete. The actual checks for the validity of the instruction is not done until installation. Best regards, Nam
On Wed, Aug 30, 2023 at 9:46 AM Nam Cao <namcaov@gmail.com> wrote: > > On Wed, Aug 30, 2023 at 9:32 AM Björn Töpel <bjorn@kernel.org> wrote: > > > > Nam Cao <namcaov@gmail.com> writes: > > > > > On Tue, Aug 29, 2023 at 08:14:59AM +0200, Björn Töpel wrote: > > >> Nam Cao <namcaov@gmail.com> writes: > > >> > > >> > On Mon, Aug 28, 2023 at 03:31:15PM +0200, Nam Cao wrote: > > >> >> On Mon, Aug 28, 2023 at 02:48:06PM +0200, Björn Töpel wrote: > > >> >> > Nam Cao <namcaov@gmail.com> writes: > > >> >> > > > >> >> > > 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 cause an infinite loop of entering > > >> >> > > and exiting trap handler. > > >> >> > > > > >> >> > > Instead of using the default implementation, implement this function > > >> >> > > speficially for riscv which checks for both ebreak and c.ebreak. > > >> >> > > > >> >> > I took this for a spin, and it indeed fixes this new hang! Nice! > > >> >> > > >> >> Great! Thanks for testing it. > > >> >> > > >> >> > However, when I tried setting an uprobe on the ebreak instruction > > >> >> > (offset 0x118) from your example [1], the probe does not show up in the > > >> >> > trace buffer. > > >> >> > > > >> >> > Any ideas? > > >> >> > > >> >> >From my understanding, both uprobes and kprobes refuse to install break points > > >> >> into existing trap instructions. Otherwise, we may conflict with something else > > >> >> that is also using trap instructions. > > >> > > > >> > I just realize you probably ask this because uprobe can still be installed before > > >> > applying the patch. But I think that is another bug that my patch also > > >> > accidentally fix: uprobes should not install breakpoint into ebreak instructions, > > >> > but it incorrectly does so because it does not even know about the existence of > > >> > 32-bit ebreak. > > >> > > >> FWIW, I can still install the uprobe at an ebreak with you patch. It's > > >> not hit, but succeeds to install. > > > > > > It seems uprobes install failures are completely silent (see uprobe_mmap() in > > > kernel/events/uprobes.c). So I think although uprobes install seems fine, it > > > actually is not. > > > > Huh, so there's no check if the instruction is a valid one at event > > register point? > > There are some checks (eg. if the probe is within the binary), but > they are not complete. Oh wait, ignore that, just tested, this is also not checked. > The actual checks for the validity of the instruction is not done > until installation. > > Best regards, > Nam
On Tue, Aug 29, 2023 at 10:18:30AM +0200, Nam Cao wrote: > Now I'm not sure what you mean. But I agree with Guo Ren here, users can use > compressed instructions but kernel does not have it enabled. So we should > always check c.ebreak regardless of RISCV_ISA_C. I think I was just being dumb, apologies for the noise.
diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c index 194f166b2cc4..91f4ce101cd1 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,15 @@ bool is_swbp_insn(uprobe_opcode_t *insn) #endif } +bool is_trap_insn(uprobe_opcode_t *insn) +{ +#ifdef CONFIG_RISCV_ISA_C + if (riscv_insn_is_c_ebreak(*insn)) + return true; +#endif + return riscv_insn_is_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 cause an infinite loop of entering and exiting trap handler. Instead of using the default implementation, implement this function speficially for riscv which checks for both ebreak and c.ebreak. Fixes: 74784081aac8 ("riscv: Add uprobes supported") Signed-off-by: Nam Cao <namcaov@gmail.com> --- arch/riscv/kernel/probes/uprobes.c | 10 ++++++++++ 1 file changed, 10 insertions(+)