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

["Liao, Chang" <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.

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