* [PATCH v2 1/8] arm64: debug: Remove unused return value from do_debug_exception()
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 2/8] arm64: debug: Rename addr parameter for non-watchpoint exception hooks Will Deacon
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
do_debug_exception() goes out of its way to return a value that isn't
ever used, so just make the thing void.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/mm/fault.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 1a7e92ab69eb..606c46aeed99 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -810,13 +810,12 @@ void __init hook_debug_fault_code(int nr,
debug_fault_info[nr].name = name;
}
-asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
- unsigned int esr,
- struct pt_regs *regs)
+asmlinkage void __exception do_debug_exception(unsigned long addr_if_watchpoint,
+ unsigned int esr,
+ struct pt_regs *regs)
{
const struct fault_info *inf = esr_to_debug_fault_info(esr);
unsigned long pc = instruction_pointer(regs);
- int rv;
/*
* Tell lockdep we disabled irqs in entry.S. Do nothing if they were
@@ -828,17 +827,12 @@ asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
if (user_mode(regs) && !is_ttbr0_addr(pc))
arm64_apply_bp_hardening();
- if (!inf->fn(addr_if_watchpoint, esr, regs)) {
- rv = 1;
- } else {
+ if (inf->fn(addr_if_watchpoint, esr, regs)) {
arm64_notify_die(inf->name, regs,
inf->sig, inf->code, (void __user *)pc, esr);
- rv = 0;
}
if (interrupts_enabled(regs))
trace_hardirqs_on();
-
- return rv;
}
NOKPROBE_SYMBOL(do_debug_exception);
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/8] arm64: debug: Rename addr parameter for non-watchpoint exception hooks
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
2019-04-05 12:40 ` [PATCH v2 1/8] arm64: debug: Remove unused return value from do_debug_exception() Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 3/8] arm64: debug: Remove meaningless comment Will Deacon
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
Since the 'addr' parameter contains an UNKNOWN value for non-watchpoint
debug exceptions, rename it to 'unused' for those hooks so we don't get
tempted to use it in the future.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/kernel/debug-monitors.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index d7bb6aefae0a..c4c263d0cf0f 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -222,7 +222,7 @@ static void send_user_sigtrap(int si_code)
"User debug trap");
}
-static int single_step_handler(unsigned long addr, unsigned int esr,
+static int single_step_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
bool handler_found = false;
@@ -302,7 +302,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
}
NOKPROBE_SYMBOL(call_break_hook);
-static int brk_handler(unsigned long addr, unsigned int esr,
+static int brk_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
bool handler_found = false;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 3/8] arm64: debug: Remove meaningless comment
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
2019-04-05 12:40 ` [PATCH v2 1/8] arm64: debug: Remove unused return value from do_debug_exception() Will Deacon
2019-04-05 12:40 ` [PATCH v2 2/8] arm64: debug: Rename addr parameter for non-watchpoint exception hooks Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 4/8] arm64: debug: Separate debug hooks based on target exception level Will Deacon
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
The comment next to the definition of our 'break_hook' list head is
at best wrong but mainly just meaningless. Rip it out.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/kernel/debug-monitors.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index c4c263d0cf0f..744229d10ca8 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -264,11 +264,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
}
NOKPROBE_SYMBOL(single_step_handler);
-/*
- * Breakpoint handler is re-entrant as another breakpoint can
- * hit within breakpoint handler, especically in kprobes.
- * Use reader/writer locks instead of plain spinlock.
- */
static LIST_HEAD(break_hook);
static DEFINE_SPINLOCK(break_hook_lock);
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 4/8] arm64: debug: Separate debug hooks based on target exception level
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
` (2 preceding siblings ...)
2019-04-05 12:40 ` [PATCH v2 3/8] arm64: debug: Remove meaningless comment Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-08 16:28 ` Mark Rutland
2019-04-05 12:40 ` [PATCH v2 5/8] arm64: kprobes: Avoid calling kprobes debug handlers explicitly Will Deacon
` (3 subsequent siblings)
7 siblings, 1 reply; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
Mixing kernel and user debug hooks together is highly error-prone as it
relies on all of the hooks to figure out whether the exception came from
kernel or user, and then to act accordingly.
Make our debug hook code a little more robust by maintaining separate
hook lists for user and kernel, with separate registration functions
to force callers to be explicit about the exception levels that they
care about.
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/include/asm/brk-imm.h | 1 +
arch/arm64/include/asm/debug-monitors.h | 18 ++++---
arch/arm64/kernel/debug-monitors.c | 85 +++++++++++++++++++++++----------
arch/arm64/kernel/kgdb.c | 22 ++++-----
arch/arm64/kernel/probes/uprobes.c | 7 ++-
arch/arm64/kernel/traps.c | 20 ++++----
6 files changed, 95 insertions(+), 58 deletions(-)
diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
index 2945fe6cd863..fec9e1384641 100644
--- a/arch/arm64/include/asm/brk-imm.h
+++ b/arch/arm64/include/asm/brk-imm.h
@@ -23,5 +23,6 @@
#define KGDB_COMPILED_DBG_BRK_IMM 0x401
#define BUG_BRK_IMM 0x800
#define KASAN_BRK_IMM 0x900
+#define KASAN_BRK_MASK 0x0ff
#endif
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index a44cf5225429..7d37cfa5cc16 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -94,18 +94,24 @@ struct step_hook {
int (*fn)(struct pt_regs *regs, unsigned int esr);
};
-void register_step_hook(struct step_hook *hook);
-void unregister_step_hook(struct step_hook *hook);
+void register_user_step_hook(struct step_hook *hook);
+void unregister_user_step_hook(struct step_hook *hook);
+
+void register_kernel_step_hook(struct step_hook *hook);
+void unregister_kernel_step_hook(struct step_hook *hook);
struct break_hook {
struct list_head node;
- u32 esr_val;
- u32 esr_mask;
int (*fn)(struct pt_regs *regs, unsigned int esr);
+ u16 imm;
+ u16 mask; /* These bits are ignored when comparing with imm */
};
-void register_break_hook(struct break_hook *hook);
-void unregister_break_hook(struct break_hook *hook);
+void register_user_break_hook(struct break_hook *hook);
+void unregister_user_break_hook(struct break_hook *hook);
+
+void register_kernel_break_hook(struct break_hook *hook);
+void unregister_kernel_break_hook(struct break_hook *hook);
u8 debug_monitors_arch(void);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 744229d10ca8..9b3fd7fa5b43 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -163,25 +163,46 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
}
NOKPROBE_SYMBOL(clear_regs_spsr_ss);
-/* EL1 Single Step Handler hooks */
-static LIST_HEAD(step_hook);
-static DEFINE_SPINLOCK(step_hook_lock);
+static DEFINE_SPINLOCK(debug_hook_lock);
+static LIST_HEAD(user_step_hook);
+static LIST_HEAD(kernel_step_hook);
-void register_step_hook(struct step_hook *hook)
+static void register_debug_hook(struct list_head *node, struct list_head *list)
{
- spin_lock(&step_hook_lock);
- list_add_rcu(&hook->node, &step_hook);
- spin_unlock(&step_hook_lock);
+ spin_lock(&debug_hook_lock);
+ list_add_rcu(node, list);
+ spin_unlock(&debug_hook_lock);
+
}
-void unregister_step_hook(struct step_hook *hook)
+static void unregister_debug_hook(struct list_head *node)
{
- spin_lock(&step_hook_lock);
- list_del_rcu(&hook->node);
- spin_unlock(&step_hook_lock);
+ spin_lock(&debug_hook_lock);
+ list_del_rcu(node);
+ spin_unlock(&debug_hook_lock);
synchronize_rcu();
}
+void register_user_step_hook(struct step_hook *hook)
+{
+ register_debug_hook(&hook->node, &user_step_hook);
+}
+
+void unregister_user_step_hook(struct step_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
+}
+
+void register_kernel_step_hook(struct step_hook *hook)
+{
+ register_debug_hook(&hook->node, &kernel_step_hook);
+}
+
+void unregister_kernel_step_hook(struct step_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
+}
+
/*
* Call registered single step handlers
* There is no Syndrome info to check for determining the handler.
@@ -191,11 +212,14 @@ void unregister_step_hook(struct step_hook *hook)
static int call_step_hook(struct pt_regs *regs, unsigned int esr)
{
struct step_hook *hook;
+ struct list_head *list;
int retval = DBG_HOOK_ERROR;
+ list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
+
rcu_read_lock();
- list_for_each_entry_rcu(hook, &step_hook, node) {
+ list_for_each_entry_rcu(hook, list, node) {
retval = hook->fn(regs, esr);
if (retval == DBG_HOOK_HANDLED)
break;
@@ -264,33 +288,44 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
}
NOKPROBE_SYMBOL(single_step_handler);
-static LIST_HEAD(break_hook);
-static DEFINE_SPINLOCK(break_hook_lock);
+static LIST_HEAD(user_break_hook);
+static LIST_HEAD(kernel_break_hook);
-void register_break_hook(struct break_hook *hook)
+void register_user_break_hook(struct break_hook *hook)
{
- spin_lock(&break_hook_lock);
- list_add_rcu(&hook->node, &break_hook);
- spin_unlock(&break_hook_lock);
+ register_debug_hook(&hook->node, &user_break_hook);
}
-void unregister_break_hook(struct break_hook *hook)
+void unregister_user_break_hook(struct break_hook *hook)
{
- spin_lock(&break_hook_lock);
- list_del_rcu(&hook->node);
- spin_unlock(&break_hook_lock);
- synchronize_rcu();
+ unregister_debug_hook(&hook->node);
+}
+
+void register_kernel_break_hook(struct break_hook *hook)
+{
+ register_debug_hook(&hook->node, &kernel_break_hook);
+}
+
+void unregister_kernel_break_hook(struct break_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
}
static int call_break_hook(struct pt_regs *regs, unsigned int esr)
{
struct break_hook *hook;
+ struct list_head *list;
int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
+ list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
+
rcu_read_lock();
- list_for_each_entry_rcu(hook, &break_hook, node)
- if ((esr & hook->esr_mask) == hook->esr_val)
+ list_for_each_entry_rcu(hook, list, node) {
+ unsigned int comment = esr & BRK64_ESR_MASK;
+
+ if ((comment & ~hook->mask) == hook->imm)
fn = hook->fn;
+ }
rcu_read_unlock();
return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 691854b77c7f..4c01f299aeb2 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -275,15 +275,13 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
NOKPROBE_SYMBOL(kgdb_step_brk_fn);
static struct break_hook kgdb_brkpt_hook = {
- .esr_mask = 0xffffffff,
- .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_DYN_DBG_BRK_IMM),
- .fn = kgdb_brk_fn
+ .fn = kgdb_brk_fn,
+ .imm = KGDB_DYN_DBG_BRK_IMM,
};
static struct break_hook kgdb_compiled_brkpt_hook = {
- .esr_mask = 0xffffffff,
- .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_COMPILED_DBG_BRK_IMM),
- .fn = kgdb_compiled_brk_fn
+ .fn = kgdb_compiled_brk_fn,
+ .imm = KGDB_COMPILED_DBG_BRK_IMM,
};
static struct step_hook kgdb_step_hook = {
@@ -332,9 +330,9 @@ int kgdb_arch_init(void)
if (ret != 0)
return ret;
- register_break_hook(&kgdb_brkpt_hook);
- register_break_hook(&kgdb_compiled_brkpt_hook);
- register_step_hook(&kgdb_step_hook);
+ register_kernel_break_hook(&kgdb_brkpt_hook);
+ register_kernel_break_hook(&kgdb_compiled_brkpt_hook);
+ register_kernel_step_hook(&kgdb_step_hook);
return 0;
}
@@ -345,9 +343,9 @@ int kgdb_arch_init(void)
*/
void kgdb_arch_exit(void)
{
- unregister_break_hook(&kgdb_brkpt_hook);
- unregister_break_hook(&kgdb_compiled_brkpt_hook);
- unregister_step_hook(&kgdb_step_hook);
+ unregister_kernel_break_hook(&kgdb_brkpt_hook);
+ unregister_kernel_break_hook(&kgdb_compiled_brkpt_hook);
+ unregister_kernel_step_hook(&kgdb_step_hook);
unregister_die_notifier(&kgdb_notifier);
}
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index 636ca0119c0e..7d6ea88796a6 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -195,8 +195,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
/* uprobe breakpoint handler hook */
static struct break_hook uprobes_break_hook = {
- .esr_mask = BRK64_ESR_MASK,
- .esr_val = BRK64_ESR_UPROBES,
+ .imm = BRK64_ESR_UPROBES,
.fn = uprobe_breakpoint_handler,
};
@@ -207,8 +206,8 @@ static struct step_hook uprobes_step_hook = {
static int __init arch_init_uprobes(void)
{
- register_break_hook(&uprobes_break_hook);
- register_step_hook(&uprobes_step_hook);
+ register_user_break_hook(&uprobes_break_hook);
+ register_user_step_hook(&uprobes_step_hook);
return 0;
}
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8ad119c3f665..85fdc3d7c556 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -969,9 +969,8 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
}
static struct break_hook bug_break_hook = {
- .esr_val = 0xf2000000 | BUG_BRK_IMM,
- .esr_mask = 0xffffffff,
.fn = bug_handler,
+ .imm = BUG_BRK_IMM,
};
#ifdef CONFIG_KASAN_SW_TAGS
@@ -1016,13 +1015,10 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
return DBG_HOOK_HANDLED;
}
-#define KASAN_ESR_VAL (0xf2000000 | KASAN_BRK_IMM)
-#define KASAN_ESR_MASK 0xffffff00
-
static struct break_hook kasan_break_hook = {
- .esr_val = KASAN_ESR_VAL,
- .esr_mask = KASAN_ESR_MASK,
- .fn = kasan_handler,
+ .fn = kasan_handler,
+ .imm = KASAN_BRK_IMM,
+ .mask = KASAN_BRK_MASK,
};
#endif
@@ -1034,7 +1030,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
struct pt_regs *regs)
{
#ifdef CONFIG_KASAN_SW_TAGS
- if ((esr & KASAN_ESR_MASK) == KASAN_ESR_VAL)
+ unsigned int comment = esr & BRK64_ESR_MASK;
+
+ if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
#endif
return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
@@ -1043,8 +1041,8 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
/* This registration must happen early, before debug_traps_init(). */
void __init trap_init(void)
{
- register_break_hook(&bug_break_hook);
+ register_kernel_break_hook(&bug_break_hook);
#ifdef CONFIG_KASAN_SW_TAGS
- register_break_hook(&kasan_break_hook);
+ register_kernel_break_hook(&kasan_break_hook);
#endif
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/8] arm64: debug: Separate debug hooks based on target exception level
2019-04-05 12:40 ` [PATCH v2 4/8] arm64: debug: Separate debug hooks based on target exception level Will Deacon
@ 2019-04-08 16:28 ` Mark Rutland
0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2019-04-08 16:28 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-arm-kernel
On Fri, Apr 05, 2019 at 01:40:44PM +0100, Will Deacon wrote:
> Mixing kernel and user debug hooks together is highly error-prone as it
> relies on all of the hooks to figure out whether the exception came from
> kernel or user, and then to act accordingly.
>
> Make our debug hook code a little more robust by maintaining separate
> hook lists for user and kernel, with separate registration functions
> to force callers to be explicit about the exception levels that they
> care about.
>
> Signed-off-by: Will Deacon <will.deacon@arm•com>
One minor comment below, but either way this looks good to me:
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
> ---
> arch/arm64/include/asm/brk-imm.h | 1 +
> arch/arm64/include/asm/debug-monitors.h | 18 ++++---
> arch/arm64/kernel/debug-monitors.c | 85 +++++++++++++++++++++++----------
> arch/arm64/kernel/kgdb.c | 22 ++++-----
> arch/arm64/kernel/probes/uprobes.c | 7 ++-
> arch/arm64/kernel/traps.c | 20 ++++----
> 6 files changed, 95 insertions(+), 58 deletions(-)
>
> diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
> index 2945fe6cd863..fec9e1384641 100644
> --- a/arch/arm64/include/asm/brk-imm.h
> +++ b/arch/arm64/include/asm/brk-imm.h
> @@ -23,5 +23,6 @@
> #define KGDB_COMPILED_DBG_BRK_IMM 0x401
> #define BUG_BRK_IMM 0x800
> #define KASAN_BRK_IMM 0x900
> +#define KASAN_BRK_MASK 0x0ff
>
> #endif
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index a44cf5225429..7d37cfa5cc16 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -94,18 +94,24 @@ struct step_hook {
> int (*fn)(struct pt_regs *regs, unsigned int esr);
> };
>
> -void register_step_hook(struct step_hook *hook);
> -void unregister_step_hook(struct step_hook *hook);
> +void register_user_step_hook(struct step_hook *hook);
> +void unregister_user_step_hook(struct step_hook *hook);
> +
> +void register_kernel_step_hook(struct step_hook *hook);
> +void unregister_kernel_step_hook(struct step_hook *hook);
>
> struct break_hook {
> struct list_head node;
> - u32 esr_val;
> - u32 esr_mask;
> int (*fn)(struct pt_regs *regs, unsigned int esr);
> + u16 imm;
> + u16 mask; /* These bits are ignored when comparing with imm */
It might make sense to call this imm_mask, but that's not a big deal
either way.
Mark.
> };
>
> -void register_break_hook(struct break_hook *hook);
> -void unregister_break_hook(struct break_hook *hook);
> +void register_user_break_hook(struct break_hook *hook);
> +void unregister_user_break_hook(struct break_hook *hook);
> +
> +void register_kernel_break_hook(struct break_hook *hook);
> +void unregister_kernel_break_hook(struct break_hook *hook);
>
> u8 debug_monitors_arch(void);
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 744229d10ca8..9b3fd7fa5b43 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -163,25 +163,46 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(clear_regs_spsr_ss);
>
> -/* EL1 Single Step Handler hooks */
> -static LIST_HEAD(step_hook);
> -static DEFINE_SPINLOCK(step_hook_lock);
> +static DEFINE_SPINLOCK(debug_hook_lock);
> +static LIST_HEAD(user_step_hook);
> +static LIST_HEAD(kernel_step_hook);
>
> -void register_step_hook(struct step_hook *hook)
> +static void register_debug_hook(struct list_head *node, struct list_head *list)
> {
> - spin_lock(&step_hook_lock);
> - list_add_rcu(&hook->node, &step_hook);
> - spin_unlock(&step_hook_lock);
> + spin_lock(&debug_hook_lock);
> + list_add_rcu(node, list);
> + spin_unlock(&debug_hook_lock);
> +
> }
>
> -void unregister_step_hook(struct step_hook *hook)
> +static void unregister_debug_hook(struct list_head *node)
> {
> - spin_lock(&step_hook_lock);
> - list_del_rcu(&hook->node);
> - spin_unlock(&step_hook_lock);
> + spin_lock(&debug_hook_lock);
> + list_del_rcu(node);
> + spin_unlock(&debug_hook_lock);
> synchronize_rcu();
> }
>
> +void register_user_step_hook(struct step_hook *hook)
> +{
> + register_debug_hook(&hook->node, &user_step_hook);
> +}
> +
> +void unregister_user_step_hook(struct step_hook *hook)
> +{
> + unregister_debug_hook(&hook->node);
> +}
> +
> +void register_kernel_step_hook(struct step_hook *hook)
> +{
> + register_debug_hook(&hook->node, &kernel_step_hook);
> +}
> +
> +void unregister_kernel_step_hook(struct step_hook *hook)
> +{
> + unregister_debug_hook(&hook->node);
> +}
> +
> /*
> * Call registered single step handlers
> * There is no Syndrome info to check for determining the handler.
> @@ -191,11 +212,14 @@ void unregister_step_hook(struct step_hook *hook)
> static int call_step_hook(struct pt_regs *regs, unsigned int esr)
> {
> struct step_hook *hook;
> + struct list_head *list;
> int retval = DBG_HOOK_ERROR;
>
> + list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
> +
> rcu_read_lock();
>
> - list_for_each_entry_rcu(hook, &step_hook, node) {
> + list_for_each_entry_rcu(hook, list, node) {
> retval = hook->fn(regs, esr);
> if (retval == DBG_HOOK_HANDLED)
> break;
> @@ -264,33 +288,44 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
> }
> NOKPROBE_SYMBOL(single_step_handler);
>
> -static LIST_HEAD(break_hook);
> -static DEFINE_SPINLOCK(break_hook_lock);
> +static LIST_HEAD(user_break_hook);
> +static LIST_HEAD(kernel_break_hook);
>
> -void register_break_hook(struct break_hook *hook)
> +void register_user_break_hook(struct break_hook *hook)
> {
> - spin_lock(&break_hook_lock);
> - list_add_rcu(&hook->node, &break_hook);
> - spin_unlock(&break_hook_lock);
> + register_debug_hook(&hook->node, &user_break_hook);
> }
>
> -void unregister_break_hook(struct break_hook *hook)
> +void unregister_user_break_hook(struct break_hook *hook)
> {
> - spin_lock(&break_hook_lock);
> - list_del_rcu(&hook->node);
> - spin_unlock(&break_hook_lock);
> - synchronize_rcu();
> + unregister_debug_hook(&hook->node);
> +}
> +
> +void register_kernel_break_hook(struct break_hook *hook)
> +{
> + register_debug_hook(&hook->node, &kernel_break_hook);
> +}
> +
> +void unregister_kernel_break_hook(struct break_hook *hook)
> +{
> + unregister_debug_hook(&hook->node);
> }
>
> static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> {
> struct break_hook *hook;
> + struct list_head *list;
> int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
>
> + list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
> +
> rcu_read_lock();
> - list_for_each_entry_rcu(hook, &break_hook, node)
> - if ((esr & hook->esr_mask) == hook->esr_val)
> + list_for_each_entry_rcu(hook, list, node) {
> + unsigned int comment = esr & BRK64_ESR_MASK;
> +
> + if ((comment & ~hook->mask) == hook->imm)
> fn = hook->fn;
> + }
> rcu_read_unlock();
>
> return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 691854b77c7f..4c01f299aeb2 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -275,15 +275,13 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
> NOKPROBE_SYMBOL(kgdb_step_brk_fn);
>
> static struct break_hook kgdb_brkpt_hook = {
> - .esr_mask = 0xffffffff,
> - .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_DYN_DBG_BRK_IMM),
> - .fn = kgdb_brk_fn
> + .fn = kgdb_brk_fn,
> + .imm = KGDB_DYN_DBG_BRK_IMM,
> };
>
> static struct break_hook kgdb_compiled_brkpt_hook = {
> - .esr_mask = 0xffffffff,
> - .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_COMPILED_DBG_BRK_IMM),
> - .fn = kgdb_compiled_brk_fn
> + .fn = kgdb_compiled_brk_fn,
> + .imm = KGDB_COMPILED_DBG_BRK_IMM,
> };
>
> static struct step_hook kgdb_step_hook = {
> @@ -332,9 +330,9 @@ int kgdb_arch_init(void)
> if (ret != 0)
> return ret;
>
> - register_break_hook(&kgdb_brkpt_hook);
> - register_break_hook(&kgdb_compiled_brkpt_hook);
> - register_step_hook(&kgdb_step_hook);
> + register_kernel_break_hook(&kgdb_brkpt_hook);
> + register_kernel_break_hook(&kgdb_compiled_brkpt_hook);
> + register_kernel_step_hook(&kgdb_step_hook);
> return 0;
> }
>
> @@ -345,9 +343,9 @@ int kgdb_arch_init(void)
> */
> void kgdb_arch_exit(void)
> {
> - unregister_break_hook(&kgdb_brkpt_hook);
> - unregister_break_hook(&kgdb_compiled_brkpt_hook);
> - unregister_step_hook(&kgdb_step_hook);
> + unregister_kernel_break_hook(&kgdb_brkpt_hook);
> + unregister_kernel_break_hook(&kgdb_compiled_brkpt_hook);
> + unregister_kernel_step_hook(&kgdb_step_hook);
> unregister_die_notifier(&kgdb_notifier);
> }
>
> diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
> index 636ca0119c0e..7d6ea88796a6 100644
> --- a/arch/arm64/kernel/probes/uprobes.c
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -195,8 +195,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
>
> /* uprobe breakpoint handler hook */
> static struct break_hook uprobes_break_hook = {
> - .esr_mask = BRK64_ESR_MASK,
> - .esr_val = BRK64_ESR_UPROBES,
> + .imm = BRK64_ESR_UPROBES,
> .fn = uprobe_breakpoint_handler,
> };
>
> @@ -207,8 +206,8 @@ static struct step_hook uprobes_step_hook = {
>
> static int __init arch_init_uprobes(void)
> {
> - register_break_hook(&uprobes_break_hook);
> - register_step_hook(&uprobes_step_hook);
> + register_user_break_hook(&uprobes_break_hook);
> + register_user_step_hook(&uprobes_step_hook);
>
> return 0;
> }
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 8ad119c3f665..85fdc3d7c556 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -969,9 +969,8 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
> }
>
> static struct break_hook bug_break_hook = {
> - .esr_val = 0xf2000000 | BUG_BRK_IMM,
> - .esr_mask = 0xffffffff,
> .fn = bug_handler,
> + .imm = BUG_BRK_IMM,
> };
>
> #ifdef CONFIG_KASAN_SW_TAGS
> @@ -1016,13 +1015,10 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
> return DBG_HOOK_HANDLED;
> }
>
> -#define KASAN_ESR_VAL (0xf2000000 | KASAN_BRK_IMM)
> -#define KASAN_ESR_MASK 0xffffff00
> -
> static struct break_hook kasan_break_hook = {
> - .esr_val = KASAN_ESR_VAL,
> - .esr_mask = KASAN_ESR_MASK,
> - .fn = kasan_handler,
> + .fn = kasan_handler,
> + .imm = KASAN_BRK_IMM,
> + .mask = KASAN_BRK_MASK,
> };
> #endif
>
> @@ -1034,7 +1030,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> {
> #ifdef CONFIG_KASAN_SW_TAGS
> - if ((esr & KASAN_ESR_MASK) == KASAN_ESR_VAL)
> + unsigned int comment = esr & BRK64_ESR_MASK;
> +
> + if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> #endif
> return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
> @@ -1043,8 +1041,8 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> /* This registration must happen early, before debug_traps_init(). */
> void __init trap_init(void)
> {
> - register_break_hook(&bug_break_hook);
> + register_kernel_break_hook(&bug_break_hook);
> #ifdef CONFIG_KASAN_SW_TAGS
> - register_break_hook(&kasan_break_hook);
> + register_kernel_break_hook(&kasan_break_hook);
> #endif
> }
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 5/8] arm64: kprobes: Avoid calling kprobes debug handlers explicitly
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
` (3 preceding siblings ...)
2019-04-05 12:40 ` [PATCH v2 4/8] arm64: debug: Separate debug hooks based on target exception level Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 6/8] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers Will Deacon
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
Kprobes bypasses our debug hook registration code so that it doesn't
get tangled up with recursive debug exceptions from things like lockdep:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/324385.html
However, since then, (a) the hook list has become RCU protected and (b)
the kprobes hooks were found not to filter out exceptions from userspace
correctly. On top of that, the step handler is invoked directly from
single_step_handler(), which *does* use the debug hook list, so it's
clearly not the end of the world.
For now, have kprobes use the debug hook registration API like everybody
else. We can revisit this in the future if this is found to limit
coverage significantly.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/include/asm/kprobes.h | 2 --
arch/arm64/kernel/debug-monitors.c | 10 ----------
arch/arm64/kernel/probes/kprobes.c | 16 ++++++++++++++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
index d5a44cf859e9..21721fbf44e7 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -54,8 +54,6 @@ void arch_remove_kprobe(struct kprobe *);
int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
int kprobe_exceptions_notify(struct notifier_block *self,
unsigned long val, void *data);
-int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
-int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
void kretprobe_trampoline(void);
void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 9b3fd7fa5b43..f4d8cda8830d 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -258,10 +258,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
if (!reinstall_suspended_bps(regs))
return 0;
-#ifdef CONFIG_KPROBES
- if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
-#endif
if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
handler_found = true;
@@ -337,12 +333,6 @@ static int brk_handler(unsigned long unused, unsigned int esr,
{
bool handler_found = false;
-#ifdef CONFIG_KPROBES
- if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
- if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
- }
-#endif
if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
handler_found = true;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 7a679caf4585..baf97f47aec0 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -439,7 +439,7 @@ kprobe_ss_hit(struct kprobe_ctlblk *kcb, unsigned long addr)
return DBG_HOOK_ERROR;
}
-int __kprobes
+static int __kprobes
kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
{
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
@@ -461,7 +461,11 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
return retval;
}
-int __kprobes
+static struct step_hook kprobes_step_hook = {
+ .fn = kprobe_single_step_handler,
+};
+
+static int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
if (user_mode(regs))
@@ -471,6 +475,11 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
return DBG_HOOK_HANDLED;
}
+static struct break_hook kprobes_break_hook = {
+ .imm = BRK64_ESR_KPROBES,
+ .fn = kprobe_breakpoint_handler,
+};
+
/*
* Provide a blacklist of symbols identifying ranges which cannot be kprobed.
* This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
@@ -599,5 +608,8 @@ int __kprobes arch_trampoline_kprobe(struct kprobe *p)
int __init arch_init_kprobes(void)
{
+ register_kernel_break_hook(&kprobes_break_hook);
+ register_kernel_step_hook(&kprobes_step_hook);
+
return 0;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 6/8] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
` (4 preceding siblings ...)
2019-04-05 12:40 ` [PATCH v2 5/8] arm64: kprobes: Avoid calling kprobes debug handlers explicitly Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 7/8] arm64: probes: Move magic BRK values into brk-imm.h Will Deacon
2019-04-05 12:40 ` [PATCH v2 8/8] arm64: debug: Clean up brk_handler() Will Deacon
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
Now that the debug hook dispatching code takes the triggering exception
level into account, there's no need for the hooks themselves to poke
around with user_mode(regs).
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/kernel/kgdb.c | 8 +-------
arch/arm64/kernel/probes/kprobes.c | 6 ------
arch/arm64/kernel/probes/uprobes.c | 12 ++++--------
arch/arm64/kernel/traps.c | 6 ------
4 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 4c01f299aeb2..30853d5b7859 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -244,9 +244,6 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kgdb_handle_exception(1, SIGTRAP, 0, regs);
return DBG_HOOK_HANDLED;
}
@@ -254,9 +251,6 @@ NOKPROBE_SYMBOL(kgdb_brk_fn)
static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
compiled_break = 1;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
@@ -266,7 +260,7 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs) || !kgdb_single_step)
+ if (!kgdb_single_step)
return DBG_HOOK_ERROR;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index baf97f47aec0..000f32d1a756 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -445,9 +445,6 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
int retval;
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
/* return error if this is not our step */
retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
@@ -468,9 +465,6 @@ static struct step_hook kprobes_step_hook = {
static int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kprobe_handler(regs);
return DBG_HOOK_HANDLED;
}
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index 7d6ea88796a6..f37ab9567676 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -171,7 +171,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self,
static int uprobe_breakpoint_handler(struct pt_regs *regs,
unsigned int esr)
{
- if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
+ if (uprobe_pre_sstep_notifier(regs))
return DBG_HOOK_HANDLED;
return DBG_HOOK_ERROR;
@@ -182,13 +182,9 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
{
struct uprobe_task *utask = current->utask;
- if (user_mode(regs)) {
- WARN_ON(utask &&
- (instruction_pointer(regs) != utask->xol_vaddr + 4));
-
- if (uprobe_post_sstep_notifier(regs))
- return DBG_HOOK_HANDLED;
- }
+ WARN_ON(utask && (instruction_pointer(regs) != utask->xol_vaddr + 4));
+ if (uprobe_post_sstep_notifier(regs))
+ return DBG_HOOK_HANDLED;
return DBG_HOOK_ERROR;
}
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 85fdc3d7c556..091379744d2f 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -947,9 +947,6 @@ int is_valid_bugaddr(unsigned long addr)
static int bug_handler(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
switch (report_bug(regs->pc, regs)) {
case BUG_TRAP_TYPE_BUG:
die("Oops - BUG", regs, 0);
@@ -988,9 +985,6 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
u64 addr = regs->regs[0];
u64 pc = regs->pc;
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kasan_report(addr, size, write, pc);
/*
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 7/8] arm64: probes: Move magic BRK values into brk-imm.h
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
` (5 preceding siblings ...)
2019-04-05 12:40 ` [PATCH v2 6/8] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
2019-04-05 12:40 ` [PATCH v2 8/8] arm64: debug: Clean up brk_handler() Will Deacon
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
kprobes and uprobes reserve some BRK immediates for installing their
probes. Define these along with the other reservations in brk-imm.h
and rename the ESR definitions to be consistent with the others that we
already have.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/include/asm/brk-imm.h | 4 ++++
arch/arm64/include/asm/debug-monitors.h | 7 ++-----
arch/arm64/include/asm/esr.h | 4 +---
arch/arm64/kernel/debug-monitors.c | 2 +-
arch/arm64/kernel/probes/kprobes.c | 2 +-
arch/arm64/kernel/probes/uprobes.c | 2 +-
arch/arm64/kernel/traps.c | 2 +-
7 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
index fec9e1384641..d84294064e6a 100644
--- a/arch/arm64/include/asm/brk-imm.h
+++ b/arch/arm64/include/asm/brk-imm.h
@@ -11,6 +11,8 @@
/*
* #imm16 values used for BRK instruction generation
+ * 0x004: for installing kprobes
+ * 0x005: for installing uprobes
* Allowed values for kgdb are 0x400 - 0x7ff
* 0x100: for triggering a fault on purpose (reserved)
* 0x400: for dynamic BRK instruction
@@ -18,6 +20,8 @@
* 0x800: kernel-mode BUG() and WARN() traps
* 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff)
*/
+#define KPROBES_BRK_IMM 0x004
+#define UPROBES_BRK_IMM 0x005
#define FAULT_BRK_IMM 0x100
#define KGDB_DYN_DBG_BRK_IMM 0x400
#define KGDB_COMPILED_DBG_BRK_IMM 0x401
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 7d37cfa5cc16..0679f781696d 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -65,12 +65,9 @@
#define CACHE_FLUSH_IS_SAFE 1
/* kprobes BRK opcodes with ESR encoding */
-#define BRK64_ESR_MASK 0xFFFF
-#define BRK64_ESR_KPROBES 0x0004
-#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
+#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (KPROBES_BRK_IMM << 5))
/* uprobes BRK opcodes with ESR encoding */
-#define BRK64_ESR_UPROBES 0x0005
-#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))
+#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (UPROBES_BRK_IMM << 5))
/* AArch32 */
#define DBG_ESR_EVT_BKPT 0x4
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 52233f00d53d..3541720189c9 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -156,9 +156,7 @@
ESR_ELx_WFx_ISS_WFI)
/* BRK instruction trap from AArch64 state */
-#define ESR_ELx_VAL_BRK64(imm) \
- ((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL | \
- ((imm) & 0xffff))
+#define ESR_ELx_BRK64_ISS_COMMENT_MASK 0xffff
/* ISS field definitions for System instruction traps */
#define ESR_ELx_SYS64_ISS_RES0_SHIFT 22
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index f4d8cda8830d..2692a0a27cf3 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -317,7 +317,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
rcu_read_lock();
list_for_each_entry_rcu(hook, list, node) {
- unsigned int comment = esr & BRK64_ESR_MASK;
+ unsigned int comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
if ((comment & ~hook->mask) == hook->imm)
fn = hook->fn;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 000f32d1a756..2509fcb6d404 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -470,7 +470,7 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
}
static struct break_hook kprobes_break_hook = {
- .imm = BRK64_ESR_KPROBES,
+ .imm = KPROBES_BRK_IMM,
.fn = kprobe_breakpoint_handler,
};
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index f37ab9567676..605945eac1f8 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -191,7 +191,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
/* uprobe breakpoint handler hook */
static struct break_hook uprobes_break_hook = {
- .imm = BRK64_ESR_UPROBES,
+ .imm = UPROBES_BRK_IMM,
.fn = uprobe_breakpoint_handler,
};
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 091379744d2f..74598396e0bf 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1024,7 +1024,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
struct pt_regs *regs)
{
#ifdef CONFIG_KASAN_SW_TAGS
- unsigned int comment = esr & BRK64_ESR_MASK;
+ unsigned int comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 8/8] arm64: debug: Clean up brk_handler()
2019-04-05 12:40 [PATCH v2 0/8] Rework debug exception handling code Will Deacon
` (6 preceding siblings ...)
2019-04-05 12:40 ` [PATCH v2 7/8] arm64: probes: Move magic BRK values into brk-imm.h Will Deacon
@ 2019-04-05 12:40 ` Will Deacon
7 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2019-04-05 12:40 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, Will Deacon
brk_handler() now looks pretty strange and can be refactored to drop its
funny 'handler_found' local variable altogether.
Reviewed-by: Mark Rutland <mark.rutland@arm•com>
Signed-off-by: Will Deacon <will.deacon@arm•com>
---
arch/arm64/kernel/debug-monitors.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 2692a0a27cf3..800486cc4823 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -331,14 +331,12 @@ NOKPROBE_SYMBOL(call_break_hook);
static int brk_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
- bool handler_found = false;
-
- if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
+ if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+ return 0;
- if (!handler_found && user_mode(regs)) {
+ if (user_mode(regs)) {
send_user_sigtrap(TRAP_BRKPT);
- } else if (!handler_found) {
+ } else {
pr_warn("Unexpected kernel BRK exception at EL1\n");
return -EFAULT;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread