linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] riscv: correct the do_trap_break()
@ 2019-09-23  0:45 Vincent Chen
  2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Vincent Chen @ 2019-09-23  0:45 UTC (permalink / raw)
  To: linux-riscv; +Cc: vincent.chen, palmer, linux-kernel, aou, paul.walmsley


The following three situations may occur in the current implementation of
do_trap_break().
1. When the CONFIG_GENERIC_BUG is disabled, if a kernel thread is trapped
   by BUG(), the whole system will be in the loop that infinitely handles
   the break exception instead of entering the die function.
2. When the kernel runs code on behalf of a user thread, and the kernel
   executes a WARN() or WARN_ON(), the user thread will be sent a bogus
   SIGTRAP.
3. Handling the unexpected ebreak instructions is to send a SIGTRAP
   to the trapped thread. However, if a kernel executes an unexpected
   ebreak, it may cause the kernel thread to be stuck in the ebreak
   instruction.

This patch set will solve the above problems by adjusting the
implementations of the do_trap_break().


Vincent Chen (4):
  riscv: avoid kernel hangs when trapped in BUG()
  rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  riscv: Correct the handling of unexpected ebreak in do_trap_break()
  riscv: remove the switch statement in do_trap_break()

 arch/riscv/kernel/traps.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG()
  2019-09-23  0:45 [PATCH 0/4] riscv: correct the do_trap_break() Vincent Chen
@ 2019-09-23  0:45 ` Vincent Chen
  2019-09-27 22:25   ` Christoph Hellwig
  2019-10-04 18:26   ` Paul Walmsley
  2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 20+ messages in thread
From: Vincent Chen @ 2019-09-23  0:45 UTC (permalink / raw)
  To: linux-riscv; +Cc: vincent.chen, palmer, linux-kernel, aou, paul.walmsley

When the CONFIG_GENERIC_BUG is disabled by disabling CONFIG_BUG, if a
kernel thread is trapped by BUG(), the whole system will be in the
loop that infinitely handles the ebreak exception instead of entering the
die function. To fix this problem, the do_trap_break() will always call
the die() to deal with the break exception as the type of break is
BUG_TRAP_TYPE_BUG.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/kernel/traps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 424eb72d56b1..055a937aca70 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -124,23 +124,23 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
 
 asmlinkage void do_trap_break(struct pt_regs *regs)
 {
-#ifdef CONFIG_GENERIC_BUG
 	if (!user_mode(regs)) {
 		enum bug_trap_type type;
 
 		type = report_bug(regs->sepc, regs);
 		switch (type) {
+#ifdef CONFIG_GENERIC_BUG
 		case BUG_TRAP_TYPE_NONE:
 			break;
 		case BUG_TRAP_TYPE_WARN:
 			regs->sepc += get_break_insn_length(regs->sepc);
 			break;
 		case BUG_TRAP_TYPE_BUG:
+#endif /* CONFIG_GENERIC_BUG */
+		default:
 			die(regs, "Kernel BUG");
 		}
 	}
-#endif /* CONFIG_GENERIC_BUG */
-
 	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc));
 }
 
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  2019-09-23  0:45 [PATCH 0/4] riscv: correct the do_trap_break() Vincent Chen
  2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
@ 2019-09-23  0:45 ` Vincent Chen
  2019-09-27 22:27   ` Christoph Hellwig
                     ` (2 more replies)
  2019-09-23  0:45 ` [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break() Vincent Chen
  2019-09-23  0:45 ` [PATCH 4/4] riscv: remove the switch statement " Vincent Chen
  3 siblings, 3 replies; 20+ messages in thread
From: Vincent Chen @ 2019-09-23  0:45 UTC (permalink / raw)
  To: linux-riscv; +Cc: vincent.chen, palmer, linux-kernel, aou, paul.walmsley

On RISC-V, when the kernel runs code on behalf of a user thread, and the
kernel executes a WARN() or WARN_ON(), the user thread will be sent
a bogus SIGTRAP.  Fix the RISC-V kernel code to not send a SIGTRAP when
a WARN()/WARN_ON() is executed.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/kernel/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 055a937aca70..82f42a55451e 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -134,7 +134,7 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 			break;
 		case BUG_TRAP_TYPE_WARN:
 			regs->sepc += get_break_insn_length(regs->sepc);
-			break;
+			return;
 		case BUG_TRAP_TYPE_BUG:
 #endif /* CONFIG_GENERIC_BUG */
 		default:
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break()
  2019-09-23  0:45 [PATCH 0/4] riscv: correct the do_trap_break() Vincent Chen
  2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
  2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
@ 2019-09-23  0:45 ` Vincent Chen
  2019-09-27 22:43   ` Christoph Hellwig
  2019-10-04 18:28   ` Paul Walmsley
  2019-09-23  0:45 ` [PATCH 4/4] riscv: remove the switch statement " Vincent Chen
  3 siblings, 2 replies; 20+ messages in thread
From: Vincent Chen @ 2019-09-23  0:45 UTC (permalink / raw)
  To: linux-riscv; +Cc: vincent.chen, palmer, linux-kernel, aou, paul.walmsley

For the kernel space, all ebreak instructions are determined at compile
time because the kernel space debugging module is currently unsupported.
Hence, it should be treated as a bug if an ebreak instruction which does
not belong to BUG_TRAP_TYPE_WARN or BUG_TRAP_TYPE_BUG is executed in
kernel space. For the userspace, debugging module or user problem may
intentionally insert an ebreak instruction to trigger a SIGTRAP signal.
To approach the above two situations, the do_trap_break() will direct
the BUG_TRAP_TYPE_NONE ebreak exception issued in kernel space to die()
and will send a SIGTRAP to the trapped process only when the ebreak is
in userspace.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/kernel/traps.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 82f42a55451e..dd13bc90aeb6 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -130,8 +130,6 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 		type = report_bug(regs->sepc, regs);
 		switch (type) {
 #ifdef CONFIG_GENERIC_BUG
-		case BUG_TRAP_TYPE_NONE:
-			break;
 		case BUG_TRAP_TYPE_WARN:
 			regs->sepc += get_break_insn_length(regs->sepc);
 			return;
@@ -140,8 +138,9 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 		default:
 			die(regs, "Kernel BUG");
 		}
-	}
-	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc));
+	} else
+		force_sig_fault(SIGTRAP, TRAP_BRKPT,
+				(void __user *)(regs->sepc));
 }
 
 #ifdef CONFIG_GENERIC_BUG
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-09-23  0:45 [PATCH 0/4] riscv: correct the do_trap_break() Vincent Chen
                   ` (2 preceding siblings ...)
  2019-09-23  0:45 ` [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break() Vincent Chen
@ 2019-09-23  0:45 ` Vincent Chen
  2019-09-27 22:47   ` Christoph Hellwig
  3 siblings, 1 reply; 20+ messages in thread
From: Vincent Chen @ 2019-09-23  0:45 UTC (permalink / raw)
  To: linux-riscv; +Cc: vincent.chen, palmer, linux-kernel, aou, paul.walmsley

To make the code more straightforward, replacing the switch statement
with if statement.

Suggested-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/kernel/traps.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index dd13bc90aeb6..1ac75f7d0bff 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -124,23 +124,24 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
 
 asmlinkage void do_trap_break(struct pt_regs *regs)
 {
-	if (!user_mode(regs)) {
+	if (user_mode(regs)) {
+		force_sig_fault(SIGTRAP, TRAP_BRKPT,
+				(void __user *)(regs->sepc));
+		return;
+	}
+#ifdef CONFIG_GENERIC_BUG
+	{
 		enum bug_trap_type type;
 
 		type = report_bug(regs->sepc, regs);
-		switch (type) {
-#ifdef CONFIG_GENERIC_BUG
-		case BUG_TRAP_TYPE_WARN:
+		if (type == BUG_TRAP_TYPE_WARN) {
 			regs->sepc += get_break_insn_length(regs->sepc);
 			return;
-		case BUG_TRAP_TYPE_BUG:
-#endif /* CONFIG_GENERIC_BUG */
-		default:
-			die(regs, "Kernel BUG");
 		}
-	} else
-		force_sig_fault(SIGTRAP, TRAP_BRKPT,
-				(void __user *)(regs->sepc));
+	}
+#endif /* CONFIG_GENERIC_BUG */
+
+	die(regs, "Kernel BUG");
 }
 
 #ifdef CONFIG_GENERIC_BUG
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG()
  2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
@ 2019-09-27 22:25   ` Christoph Hellwig
  2019-10-04 18:26   ` Paul Walmsley
  1 sibling, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-09-27 22:25 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou, paul.walmsley

On Mon, Sep 23, 2019 at 08:45:14AM +0800, Vincent Chen wrote:
> When the CONFIG_GENERIC_BUG is disabled by disabling CONFIG_BUG, if a
> kernel thread is trapped by BUG(), the whole system will be in the
> loop that infinitely handles the ebreak exception instead of entering the
> die function. To fix this problem, the do_trap_break() will always call
> the die() to deal with the break exception as the type of break is
> BUG_TRAP_TYPE_BUG.
> 
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
@ 2019-09-27 22:27   ` Christoph Hellwig
  2019-09-27 22:56   ` Christoph Hellwig
  2019-10-04 18:26   ` Paul Walmsley
  2 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-09-27 22:27 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou, paul.walmsley

On Mon, Sep 23, 2019 at 08:45:15AM +0800, Vincent Chen wrote:
> On RISC-V, when the kernel runs code on behalf of a user thread, and the
> kernel executes a WARN() or WARN_ON(), the user thread will be sent
> a bogus SIGTRAP.  Fix the RISC-V kernel code to not send a SIGTRAP when
> a WARN()/WARN_ON() is executed.
> 
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break()
  2019-09-23  0:45 ` [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break() Vincent Chen
@ 2019-09-27 22:43   ` Christoph Hellwig
  2019-10-04 18:28   ` Paul Walmsley
  1 sibling, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-09-27 22:43 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou, paul.walmsley

Looks ok:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-09-23  0:45 ` [PATCH 4/4] riscv: remove the switch statement " Vincent Chen
@ 2019-09-27 22:47   ` Christoph Hellwig
  2019-10-07 16:08     ` Paul Walmsley
  0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2019-09-27 22:47 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou, paul.walmsley

On Mon, Sep 23, 2019 at 08:45:17AM +0800, Vincent Chen wrote:
> To make the code more straightforward, replacing the switch statement
> with if statement.
> 
> Suggested-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/kernel/traps.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index dd13bc90aeb6..1ac75f7d0bff 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -124,23 +124,24 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
>  
>  asmlinkage void do_trap_break(struct pt_regs *regs)
>  {
> +	if (user_mode(regs)) {
> +		force_sig_fault(SIGTRAP, TRAP_BRKPT,
> +				(void __user *)(regs->sepc));
> +		return;
> +	}
> +#ifdef CONFIG_GENERIC_BUG
> +	{
>  		enum bug_trap_type type;
>  
>  		type = report_bug(regs->sepc, regs);
> +		if (type == BUG_TRAP_TYPE_WARN) {
>  			regs->sepc += get_break_insn_length(regs->sepc);
>  			return;
>  		}
> +#endif /* CONFIG_GENERIC_BUG */
> +
> +	die(regs, "Kernel BUG");

I like where this is going, but I think this can be improved further
given that fact that report_bug has a nice stub for the
!CONFIG_GENERIC_BUG case.

How about:

	if (user_mode(regs))
		force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->sepc);
	else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN)
		regs->sepc += get_break_insn_length(regs->sepc);
	else
		die(regs, "Kernel BUG");

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
  2019-09-27 22:27   ` Christoph Hellwig
@ 2019-09-27 22:56   ` Christoph Hellwig
  2019-09-30  0:22     ` Vincent Chen
  2019-10-04 18:26   ` Paul Walmsley
  2 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2019-09-27 22:56 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou, paul.walmsley

Oh and s/rsicv/riscv/ in the subject, please.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  2019-09-27 22:56   ` Christoph Hellwig
@ 2019-09-30  0:22     ` Vincent Chen
  0 siblings, 0 replies; 20+ messages in thread
From: Vincent Chen @ 2019-09-30  0:22 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-riscv, Palmer Dabbelt, linux-kernel@vger.kernel.org List,
	Albert Ou, Paul Walmsley

On Sat, Sep 28, 2019 at 6:56 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> Oh and s/rsicv/riscv/ in the subject, please.

Oh! Thank you for finding this typo.
I will correct it.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG()
  2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
  2019-09-27 22:25   ` Christoph Hellwig
@ 2019-10-04 18:26   ` Paul Walmsley
  1 sibling, 0 replies; 20+ messages in thread
From: Paul Walmsley @ 2019-10-04 18:26 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou

On Mon, 23 Sep 2019, Vincent Chen wrote:

> When the CONFIG_GENERIC_BUG is disabled by disabling CONFIG_BUG, if a
> kernel thread is trapped by BUG(), the whole system will be in the
> loop that infinitely handles the ebreak exception instead of entering the
> die function. To fix this problem, the do_trap_break() will always call
> the die() to deal with the break exception as the type of break is
> BUG_TRAP_TYPE_BUG.
> 
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

Thanks, queued for v5.4-rc.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN()
  2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
  2019-09-27 22:27   ` Christoph Hellwig
  2019-09-27 22:56   ` Christoph Hellwig
@ 2019-10-04 18:26   ` Paul Walmsley
  2 siblings, 0 replies; 20+ messages in thread
From: Paul Walmsley @ 2019-10-04 18:26 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou

On Mon, 23 Sep 2019, Vincent Chen wrote:

> On RISC-V, when the kernel runs code on behalf of a user thread, and the
> kernel executes a WARN() or WARN_ON(), the user thread will be sent
> a bogus SIGTRAP.  Fix the RISC-V kernel code to not send a SIGTRAP when
> a WARN()/WARN_ON() is executed.
> 
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

Thanks, queued for v5.4-rc.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break()
  2019-09-23  0:45 ` [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break() Vincent Chen
  2019-09-27 22:43   ` Christoph Hellwig
@ 2019-10-04 18:28   ` Paul Walmsley
  1 sibling, 0 replies; 20+ messages in thread
From: Paul Walmsley @ 2019-10-04 18:28 UTC (permalink / raw)
  To: Vincent Chen; +Cc: linux-riscv, palmer, linux-kernel, aou

On Mon, 23 Sep 2019, Vincent Chen wrote:

> For the kernel space, all ebreak instructions are determined at compile
> time because the kernel space debugging module is currently unsupported.
> Hence, it should be treated as a bug if an ebreak instruction which does
> not belong to BUG_TRAP_TYPE_WARN or BUG_TRAP_TYPE_BUG is executed in
> kernel space. For the userspace, debugging module or user problem may
> intentionally insert an ebreak instruction to trigger a SIGTRAP signal.
> To approach the above two situations, the do_trap_break() will direct
> the BUG_TRAP_TYPE_NONE ebreak exception issued in kernel space to die()
> and will send a SIGTRAP to the trapped process only when the ebreak is
> in userspace.
> 
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

Thanks, queued the following for v5.4-rc.


- Paul

From: Vincent Chen <vincent.chen@sifive.com>
Date: Mon, 23 Sep 2019 08:45:16 +0800
Subject: [PATCH] riscv: Correct the handling of unexpected ebreak in
 do_trap_break()

For the kernel space, all ebreak instructions are determined at compile
time because the kernel space debugging module is currently unsupported.
Hence, it should be treated as a bug if an ebreak instruction which does
not belong to BUG_TRAP_TYPE_WARN or BUG_TRAP_TYPE_BUG is executed in
kernel space. For the userspace, debugging module or user problem may
intentionally insert an ebreak instruction to trigger a SIGTRAP signal.
To approach the above two situations, the do_trap_break() will direct
the BUG_TRAP_TYPE_NONE ebreak exception issued in kernel space to die()
and will send a SIGTRAP to the trapped process only when the ebreak is
in userspace.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
[paul.walmsley@sifive.com: fixed checkpatch issue]
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
---
 arch/riscv/kernel/traps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 82f42a55451e..93742df9067f 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -130,8 +130,6 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 		type = report_bug(regs->sepc, regs);
 		switch (type) {
 #ifdef CONFIG_GENERIC_BUG
-		case BUG_TRAP_TYPE_NONE:
-			break;
 		case BUG_TRAP_TYPE_WARN:
 			regs->sepc += get_break_insn_length(regs->sepc);
 			return;
@@ -140,8 +138,10 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 		default:
 			die(regs, "Kernel BUG");
 		}
+	} else {
+		force_sig_fault(SIGTRAP, TRAP_BRKPT,
+				(void __user *)(regs->sepc));
 	}
-	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc));
 }
 
 #ifdef CONFIG_GENERIC_BUG
-- 
2.23.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-09-27 22:47   ` Christoph Hellwig
@ 2019-10-07 16:08     ` Paul Walmsley
  2019-10-07 16:10       ` Christoph Hellwig
  0 siblings, 1 reply; 20+ messages in thread
From: Paul Walmsley @ 2019-10-07 16:08 UTC (permalink / raw)
  To: Vincent Chen; +Cc: Christoph Hellwig, linux-riscv, palmer, linux-kernel, aou

Vincent,

On Fri, 27 Sep 2019, Christoph Hellwig wrote:

> On Mon, Sep 23, 2019 at 08:45:17AM +0800, Vincent Chen wrote:
> > To make the code more straightforward, replacing the switch statement
> > with if statement.
> > 
> > Suggested-by: Paul Walmsley <paul.walmsley@sifive.com>
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

...

> I like where this is going, but I think this can be improved further
> given that fact that report_bug has a nice stub for the
> !CONFIG_GENERIC_BUG case.
> 
> How about:
> 
> 	if (user_mode(regs))
> 		force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->sepc);
> 	else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN)
> 		regs->sepc += get_break_insn_length(regs->sepc);
> 	else
> 		die(regs, "Kernel BUG");
> 

Christoph's suggestion looks good to me.  What do you think about this 
modification to your patch?

- Paul


From: Vincent Chen <vincent.chen@sifive.com>
Date: Mon, 23 Sep 2019 08:45:17 +0800
Subject: [PATCH] riscv: remove the switch statement in do_trap_break()

To make the code more straightforward, replace the switch statement
with an if statement.

Suggested-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
[paul.walmsley@sifive.com: removed CONFIG_GENERIC_BUG tests per
 Christoph's suggestion; cleaned up patch description]
Cc: Christoph Hellwig <hch@lst.de>
Link: https://lore.kernel.org/linux-riscv/20190927224711.GI4700@infradead.org/
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
---
 arch/riscv/kernel/traps.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 93742df9067f..45b82be00714 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -124,24 +124,13 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
 
 asmlinkage void do_trap_break(struct pt_regs *regs)
 {
-	if (!user_mode(regs)) {
-		enum bug_trap_type type;
-
-		type = report_bug(regs->sepc, regs);
-		switch (type) {
-#ifdef CONFIG_GENERIC_BUG
-		case BUG_TRAP_TYPE_WARN:
-			regs->sepc += get_break_insn_length(regs->sepc);
-			return;
-		case BUG_TRAP_TYPE_BUG:
-#endif /* CONFIG_GENERIC_BUG */
-		default:
-			die(regs, "Kernel BUG");
-		}
-	} else {
+	if (user_mode(regs))
 		force_sig_fault(SIGTRAP, TRAP_BRKPT,
 				(void __user *)(regs->sepc));
-	}
+	else if (report_bug(regs->sepc, regs) == BUG_TRAP_TYPE_WARN)
+		regs->sepc += get_break_insn_length(regs->sepc);
+	else
+		die(regs, "Kernel BUG");
 }
 
 #ifdef CONFIG_GENERIC_BUG
-- 
2.23.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-10-07 16:08     ` Paul Walmsley
@ 2019-10-07 16:10       ` Christoph Hellwig
  2019-10-07 16:30         ` Paul Walmsley
  2019-10-09 22:26         ` Paul Walmsley
  0 siblings, 2 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-10-07 16:10 UTC (permalink / raw)
  To: Paul Walmsley
  Cc: aou, palmer, linux-kernel, Christoph Hellwig, Vincent Chen, linux-riscv

On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote:
>  		force_sig_fault(SIGTRAP, TRAP_BRKPT,
>  				(void __user *)(regs->sepc));

No nee for the extra braces, which also means it all fits onto a single
line.  You could have just copied what I pasted..

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-10-07 16:10       ` Christoph Hellwig
@ 2019-10-07 16:30         ` Paul Walmsley
  2019-10-08  0:14           ` Vincent Chen
  2019-10-09 22:26         ` Paul Walmsley
  1 sibling, 1 reply; 20+ messages in thread
From: Paul Walmsley @ 2019-10-07 16:30 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Vincent Chen, linux-riscv, palmer, linux-kernel, aou

On Mon, 7 Oct 2019, Christoph Hellwig wrote:

> On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote:
> >  		force_sig_fault(SIGTRAP, TRAP_BRKPT,
> >  				(void __user *)(regs->sepc));
> 
> No nee for the extra braces, which also means it all fits onto a single
> line.  

Good point, will drop the extra parens and remove the braces.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-10-07 16:30         ` Paul Walmsley
@ 2019-10-08  0:14           ` Vincent Chen
  2019-10-08 17:07             ` Paul Walmsley
  0 siblings, 1 reply; 20+ messages in thread
From: Vincent Chen @ 2019-10-08  0:14 UTC (permalink / raw)
  To: Paul Walmsley
  Cc: Christoph Hellwig, linux-riscv, Palmer Dabbelt,
	linux-kernel@vger.kernel.org List, Albert Ou

Sorry,  I missed the comment. Christoph's suggestion is also good to me.
I will modify it as you suggested.
Thanks

On Tue, Oct 8, 2019 at 12:31 AM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Mon, 7 Oct 2019, Christoph Hellwig wrote:
>
> > On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote:
> > >             force_sig_fault(SIGTRAP, TRAP_BRKPT,
> > >                             (void __user *)(regs->sepc));
> >
> > No nee for the extra braces, which also means it all fits onto a single
> > line.
>
> Good point, will drop the extra parens and remove the braces.
>
>
> - Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-10-08  0:14           ` Vincent Chen
@ 2019-10-08 17:07             ` Paul Walmsley
  0 siblings, 0 replies; 20+ messages in thread
From: Paul Walmsley @ 2019-10-08 17:07 UTC (permalink / raw)
  To: Vincent Chen
  Cc: Albert Ou, Palmer Dabbelt, linux-kernel@vger.kernel.org List,
	Christoph Hellwig, Paul Walmsley, linux-riscv

On Tue, 8 Oct 2019, Vincent Chen wrote:

> Sorry,  I missed the comment. Christoph's suggestion is also good to me.
> I will modify it as you suggested.

Thanks - no need to resend, I'll queue the modified patch up here.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/4] riscv: remove the switch statement in do_trap_break()
  2019-10-07 16:10       ` Christoph Hellwig
  2019-10-07 16:30         ` Paul Walmsley
@ 2019-10-09 22:26         ` Paul Walmsley
  1 sibling, 0 replies; 20+ messages in thread
From: Paul Walmsley @ 2019-10-09 22:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Vincent Chen, linux-riscv, palmer, linux-kernel, aou

On Mon, 7 Oct 2019, Christoph Hellwig wrote:

> On Mon, Oct 07, 2019 at 09:08:23AM -0700, Paul Walmsley wrote:
> >  		force_sig_fault(SIGTRAP, TRAP_BRKPT,
> >  				(void __user *)(regs->sepc));
> 
> No nee for the extra braces, which also means it all fits onto a single
> line.  You could have just copied what I pasted..

It turns out that the rewrite breaks allnoconfig:

https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/VDCU2WOB6KQISREO4V5DTXEI2M7VOV55/

Am just going to pick up Vincent's original patch.  Then we can do 
any subsequent cleanup in a separate patch.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-10-09 22:26 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-23  0:45 [PATCH 0/4] riscv: correct the do_trap_break() Vincent Chen
2019-09-23  0:45 ` [PATCH 1/4] riscv: avoid kernel hangs when trapped in BUG() Vincent Chen
2019-09-27 22:25   ` Christoph Hellwig
2019-10-04 18:26   ` Paul Walmsley
2019-09-23  0:45 ` [PATCH 2/4] rsicv: avoid sending a SIGTRAP to a user thread trapped in WARN() Vincent Chen
2019-09-27 22:27   ` Christoph Hellwig
2019-09-27 22:56   ` Christoph Hellwig
2019-09-30  0:22     ` Vincent Chen
2019-10-04 18:26   ` Paul Walmsley
2019-09-23  0:45 ` [PATCH 3/4] riscv: Correct the handling of unexpected ebreak in do_trap_break() Vincent Chen
2019-09-27 22:43   ` Christoph Hellwig
2019-10-04 18:28   ` Paul Walmsley
2019-09-23  0:45 ` [PATCH 4/4] riscv: remove the switch statement " Vincent Chen
2019-09-27 22:47   ` Christoph Hellwig
2019-10-07 16:08     ` Paul Walmsley
2019-10-07 16:10       ` Christoph Hellwig
2019-10-07 16:30         ` Paul Walmsley
2019-10-08  0:14           ` Vincent Chen
2019-10-08 17:07             ` Paul Walmsley
2019-10-09 22:26         ` Paul Walmsley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).