[PATCH] arm64: Return early when break handler is found on linked-list

[PATCH] arm64: Return early when break handler is found on linked-list

[Liao Chang]

The search for breakpoint handlers iterate through the entire
linked list. Given that all registered hook has a valid fn field, and no
registered hooks share the same mask and imm. This commit optimize the
efficiency slightly by returning early as a matching handler is found.

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 arch/arm64/kernel/debug-monitors.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 024a7b245056..fc998956f44c 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
 
 void register_user_break_hook(struct break_hook *hook)
 {
+	WARN_ON(!hook->fn);
 	register_debug_hook(&hook->node, &user_break_hook);
 }
 
@@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
 
 void register_kernel_break_hook(struct break_hook *hook)
 {
+	WARN_ON(!hook->fn);
 	register_debug_hook(&hook->node, &kernel_break_hook);
 }
 
@@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 {
 	struct break_hook *hook;
 	struct list_head *list;
-	int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL;
 
 	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
 
@@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 	 */
 	list_for_each_entry_rcu(hook, list, node) {
 		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
-			fn = hook->fn;
+			return hook->fn(regs, esr);
 	}
 
-	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
+	return DBG_HOOK_ERROR;
 }
 NOKPROBE_SYMBOL(call_break_hook);
 
-- 
2.34.1

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Will Deacon]

On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote:
> The search for breakpoint handlers iterate through the entire
> linked list. Given that all registered hook has a valid fn field, and no
> registered hooks share the same mask and imm. This commit optimize the
> efficiency slightly by returning early as a matching handler is found.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 024a7b245056..fc998956f44c 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
>  
>  void register_user_break_hook(struct break_hook *hook)
>  {
> +	WARN_ON(!hook->fn);
>  	register_debug_hook(&hook->node, &user_break_hook);
>  }
>  
> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
>  
>  void register_kernel_break_hook(struct break_hook *hook)
>  {
> +	WARN_ON(!hook->fn);
>  	register_debug_hook(&hook->node, &kernel_break_hook);
>  }

I don't think we need these WARN_ON()s. This API is pretty limited and
passing a NULL callback doesn't make sense.

Rest of the patch looks fine.

Will

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Liao, Chang]

在 2024/8/27 20:30, Will Deacon 写道:
> On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote:
>> The search for breakpoint handlers iterate through the entire
>> linked list. Given that all registered hook has a valid fn field, and no
>> registered hooks share the same mask and imm. This commit optimize the
>> efficiency slightly by returning early as a matching handler is found.
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>> ---
>>  arch/arm64/kernel/debug-monitors.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 024a7b245056..fc998956f44c 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
>>  
>>  void register_user_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &user_break_hook);
>>  }
>>  
>> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
>>  
>>  void register_kernel_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &kernel_break_hook);
>>  }
> 
> I don't think we need these WARN_ON()s. This API is pretty limited and
> passing a NULL callback doesn't make sense.

Them will be removed in next revision.

> 
> Rest of the patch looks fine.
> 
> Will
> 
> 

-- 
BR
Liao, Chang

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Mark Rutland]

On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote:
> The search for breakpoint handlers iterate through the entire
> linked list. Given that all registered hook has a valid fn field, and no
> registered hooks share the same mask and imm. This commit optimize the
> efficiency slightly by returning early as a matching handler is found.
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>

This looks fine, though I'd love if we could clean this up to remove the
linked list entirely by separating the user/kernel entrypoints and using
a switch statement to decide the handler based on the immediate. That'd
also remove the need for RCU protection.

Last I looked that would require some largely mechanical restructuring,
and the only painful bit was the hooks that KGDB uses, since those are
the only ones that actually get unregistered.

Mark.

> ---
>  arch/arm64/kernel/debug-monitors.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 024a7b245056..fc998956f44c 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
>  
>  void register_user_break_hook(struct break_hook *hook)
>  {
> +	WARN_ON(!hook->fn);
>  	register_debug_hook(&hook->node, &user_break_hook);
>  }
>  
> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
>  
>  void register_kernel_break_hook(struct break_hook *hook)
>  {
> +	WARN_ON(!hook->fn);
>  	register_debug_hook(&hook->node, &kernel_break_hook);
>  }
>  
> @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>  {
>  	struct break_hook *hook;
>  	struct list_head *list;
> -	int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL;
>  
>  	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
>  
> @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>  	 */
>  	list_for_each_entry_rcu(hook, list, node) {
>  		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
> -			fn = hook->fn;
> +			return hook->fn(regs, esr);
>  	}
>  
> -	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
> +	return DBG_HOOK_ERROR;
>  }
>  NOKPROBE_SYMBOL(call_break_hook);
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Liao, Chang]

在 2024/8/27 20:50, Mark Rutland 写道:
> On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote:
>> The search for breakpoint handlers iterate through the entire
>> linked list. Given that all registered hook has a valid fn field, and no
>> registered hooks share the same mask and imm. This commit optimize the
>> efficiency slightly by returning early as a matching handler is found.
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> 
> This looks fine, though I'd love if we could clean this up to remove the
> linked list entirely by separating the user/kernel entrypoints and using
> a switch statement to decide the handler based on the immediate. That'd
> also remove the need for RCU protection.

Perhaps I could consider a similar approach to the bad addressing exception
in the file arch/arm64/mm/fault.c. It involves defining an array of break
hooks, including some default placeholder hooks. Kprobe, uprobe and KGDB could
then reuse existing register API to replace atomically these placeholder with
specific break hooks.

While most break hooks use the default mask for immediate checking in ESR,
with exception like KASAN and UBSAN. Then some hard-coded checks will be
used in the default base of switch statement for KASAN and UBSAN. That might
be a question.

> 
> Last I looked that would require some largely mechanical restructuring,
> and the only painful bit was the hooks that KGDB uses, since those are
> the only ones that actually get unregistered.

Unregistered hooks are repalced automically with the default placeholder hook
that returns DGB_HOOK_ERROR.

Chang.

> 
> Mark.
> 
>> ---
>>  arch/arm64/kernel/debug-monitors.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 024a7b245056..fc998956f44c 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
>>  
>>  void register_user_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &user_break_hook);
>>  }
>>  
>> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
>>  
>>  void register_kernel_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &kernel_break_hook);
>>  }
>>  
>> @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>>  {
>>  	struct break_hook *hook;
>>  	struct list_head *list;
>> -	int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL;
>>  
>>  	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
>>  
>> @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>>  	 */
>>  	list_for_each_entry_rcu(hook, list, node) {
>>  		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
>> -			fn = hook->fn;
>> +			return hook->fn(regs, esr);
>>  	}
>>  
>> -	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
>> +	return DBG_HOOK_ERROR;
>>  }
>>  NOKPROBE_SYMBOL(call_break_hook);
>>  
>> -- 
>> 2.34.1
>>
>>
> 

-- 
BR
Liao, Chang

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Liao, Chang]

Hi, Mark and Will

Kindly ping.

I'd like to get your input on the next step. Should I send a v2 patch
that only removes the WARN_ON() as Will suggested? Or should I push
forward the refactoring of calling BRK hook [1].

Thanks.

[1] https://lore.kernel.org/all/20240906031930.746118-1-liaochang1@huawei.com/

在 2024/8/27 20:50, Mark Rutland 写道:
> On Tue, Aug 27, 2024 at 11:00:46AM +0000, Liao Chang wrote:
>> The search for breakpoint handlers iterate through the entire
>> linked list. Given that all registered hook has a valid fn field, and no
>> registered hooks share the same mask and imm. This commit optimize the
>> efficiency slightly by returning early as a matching handler is found.
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> 
> This looks fine, though I'd love if we could clean this up to remove the
> linked list entirely by separating the user/kernel entrypoints and using
> a switch statement to decide the handler based on the immediate. That'd
> also remove the need for RCU protection.
> 
> Last I looked that would require some largely mechanical restructuring,
> and the only painful bit was the hooks that KGDB uses, since those are
> the only ones that actually get unregistered.
> 
> Mark.
> 
>> ---
>>  arch/arm64/kernel/debug-monitors.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 024a7b245056..fc998956f44c 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -281,6 +281,7 @@ static LIST_HEAD(kernel_break_hook);
>>  
>>  void register_user_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &user_break_hook);
>>  }
>>  
>> @@ -291,6 +292,7 @@ void unregister_user_break_hook(struct break_hook *hook)
>>  
>>  void register_kernel_break_hook(struct break_hook *hook)
>>  {
>> +	WARN_ON(!hook->fn);
>>  	register_debug_hook(&hook->node, &kernel_break_hook);
>>  }
>>  
>> @@ -303,7 +305,6 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>>  {
>>  	struct break_hook *hook;
>>  	struct list_head *list;
>> -	int (*fn)(struct pt_regs *regs, unsigned long esr) = NULL;
>>  
>>  	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
>>  
>> @@ -313,10 +314,10 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
>>  	 */
>>  	list_for_each_entry_rcu(hook, list, node) {
>>  		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
>> -			fn = hook->fn;
>> +			return hook->fn(regs, esr);
>>  	}
>>  
>> -	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
>> +	return DBG_HOOK_ERROR;
>>  }
>>  NOKPROBE_SYMBOL(call_break_hook);
>>  
>> -- 
>> 2.34.1
>>
>>
> 

-- 
BR
Liao, Chang

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Will Deacon]

On Wed, Oct 23, 2024 at 09:52:27AM +0800, Liao, Chang wrote:
> Kindly ping.
> 
> I'd like to get your input on the next step. Should I send a v2 patch
> that only removes the WARN_ON() as Will suggested? Or should I push
> forward the refactoring of calling BRK hook [1].

If you have the bandwidth to tackle Mark's refactoring idea, then please
feel free to send a series. Otherwise, I'm happy to merge the v2. We
could even do both.

Will

Re: [PATCH] arm64: Return early when break handler is found on linked-list

[Liao, Chang]

在 2024/10/24 0:40, Will Deacon 写道:
> On Wed, Oct 23, 2024 at 09:52:27AM +0800, Liao, Chang wrote:
>> Kindly ping.
>>
>> I'd like to get your input on the next step. Should I send a v2 patch
>> that only removes the WARN_ON() as Will suggested? Or should I push
>> forward the refactoring of calling BRK hook [1].
> 
> If you have the bandwidth to tackle Mark's refactoring idea, then please
> feel free to send a series. Otherwise, I'm happy to merge the v2. We
> could even do both.

Will, please pick the v2 from here:

  https://lore.kernel.org/all/20241024034120.3814224-1-liaochang1@huawei.com/

Meanwhile, I will discuss the refactoring patch with Mark. Thanks for your
suggestion.

> 
> Will

-- 
BR
Liao, Chang