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
+	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 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
>


-- 
Best Regards
 Guo Ren

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? 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 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.

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

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

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 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