[PATCH] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[PATCH] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[Bill Tsui]

In hw_break_set(), those functions actually can be combined into one.
This eliminates intermediate calls to modify_user_hw_breakpoint() that
may leave hardware registers in a partially updated state.

This redundancy was originally found in ARM ptrace, where it caused
non-4-byte address alignment checks to fail.

Link:
https://lore.kernel.org/all/20251018133731.42505-2-b10902118@ntu.edu.tw/

The LoongArch implementation appears to have been derived from ARM,
so this refactor helps avoid the same issue and simplifies future
maintenance.

Signed-off-by: Bill Tsui <b10902118@ntu.edu.tw>
---
 arch/loongarch/kernel/ptrace.c | 69 +++++++++-------------------------
 1 file changed, 17 insertions(+), 52 deletions(-)

diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index 8edd0954e55a..c6cd51569a64 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -581,13 +581,15 @@ static int ptrace_hbp_get_addr(unsigned int note_type,
 	return 0;
 }
 
-static int ptrace_hbp_set_ctrl(unsigned int note_type,
+static int ptrace_hbp_set(unsigned int note_type,
 			       struct task_struct *tsk,
-			       unsigned long idx, u32 uctrl)
+			       unsigned long idx, u64 addr,
+				   u64 mask, u32 uctrl)
 {
 	int err;
 	struct perf_event *bp;
 	struct perf_event_attr attr;
+	struct arch_hw_breakpoint *info;
 	struct arch_hw_breakpoint_ctrl ctrl;
 	struct thread_info *ti = task_thread_info(tsk);
 
@@ -597,6 +599,17 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 
 	attr = bp->attr;
 
+	/* addr */
+	/* Kernel-space address cannot be monitored by user-space */
+	if ((unsigned long)addr >= XKPRANGE)
+		return -EINVAL;
+	attr.bp_addr = addr;
+
+	/* mask */
+	info = counter_arch_bp(bp);
+	info->mask = mask;
+
+	/* ctrl */
 	switch (note_type) {
 	case NT_LOONGARCH_HW_BREAK:
 		ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
@@ -623,46 +636,6 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 	return modify_user_hw_breakpoint(bp, &attr);
 }
 
-static int ptrace_hbp_set_mask(unsigned int note_type,
-			       struct task_struct *tsk,
-			       unsigned long idx, u64 mask)
-{
-	struct perf_event *bp;
-	struct perf_event_attr attr;
-	struct arch_hw_breakpoint *info;
-
-	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp))
-		return PTR_ERR(bp);
-
-	attr = bp->attr;
-	info = counter_arch_bp(bp);
-	info->mask = mask;
-
-	return modify_user_hw_breakpoint(bp, &attr);
-}
-
-static int ptrace_hbp_set_addr(unsigned int note_type,
-			       struct task_struct *tsk,
-			       unsigned long idx, u64 addr)
-{
-	struct perf_event *bp;
-	struct perf_event_attr attr;
-
-	/* Kernel-space address cannot be monitored by user-space */
-	if ((unsigned long)addr >= XKPRANGE)
-		return -EINVAL;
-
-	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp))
-		return PTR_ERR(bp);
-
-	attr = bp->attr;
-	attr.bp_addr = addr;
-
-	return modify_user_hw_breakpoint(bp, &attr);
-}
-
 #define PTRACE_HBP_ADDR_SZ	sizeof(u64)
 #define PTRACE_HBP_MASK_SZ	sizeof(u64)
 #define PTRACE_HBP_CTRL_SZ	sizeof(u32)
@@ -733,10 +706,6 @@ static int hw_break_set(struct task_struct *target,
 					 offset, offset + PTRACE_HBP_ADDR_SZ);
 		if (ret)
 			return ret;
-
-		ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
-		if (ret)
-			return ret;
 		offset += PTRACE_HBP_ADDR_SZ;
 
 		if (!count)
@@ -746,21 +715,17 @@ static int hw_break_set(struct task_struct *target,
 					 offset, offset + PTRACE_HBP_MASK_SZ);
 		if (ret)
 			return ret;
-
-		ret = ptrace_hbp_set_mask(note_type, target, idx, mask);
-		if (ret)
-			return ret;
 		offset += PTRACE_HBP_MASK_SZ;
 
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ctrl,
 					 offset, offset + PTRACE_HBP_CTRL_SZ);
 		if (ret)
 			return ret;
+		offset += PTRACE_HBP_CTRL_SZ;
 
-		ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
+		ret = ptrace_hbp_set(note_type, target, idx, addr, mask, ctrl);
 		if (ret)
 			return ret;
-		offset += PTRACE_HBP_CTRL_SZ;
 
 		user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
 					  offset, offset + PTRACE_HBP_PAD_SZ);
-- 
2.51.0

Re: [PATCH] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[Huacai Chen]

Hi, Tiezhu,

On Wed, Oct 29, 2025 at 11:38 PM Bill Tsui <b10902118@ntu.edu.tw> wrote:
>
> In hw_break_set(), those functions actually can be combined into one.
> This eliminates intermediate calls to modify_user_hw_breakpoint() that
> may leave hardware registers in a partially updated state.
>
> This redundancy was originally found in ARM ptrace, where it caused
> non-4-byte address alignment checks to fail.
>
> Link:
> https://lore.kernel.org/all/20251018133731.42505-2-b10902118@ntu.edu.tw/
>
> The LoongArch implementation appears to have been derived from ARM,
> so this refactor helps avoid the same issue and simplifies future
> maintenance.
What do you think about it?

Huacai

>
> Signed-off-by: Bill Tsui <b10902118@ntu.edu.tw>
> ---
>  arch/loongarch/kernel/ptrace.c | 69 +++++++++-------------------------
>  1 file changed, 17 insertions(+), 52 deletions(-)
>
> diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
> index 8edd0954e55a..c6cd51569a64 100644
> --- a/arch/loongarch/kernel/ptrace.c
> +++ b/arch/loongarch/kernel/ptrace.c
> @@ -581,13 +581,15 @@ static int ptrace_hbp_get_addr(unsigned int note_type,
>         return 0;
>  }
>
> -static int ptrace_hbp_set_ctrl(unsigned int note_type,
> +static int ptrace_hbp_set(unsigned int note_type,
>                                struct task_struct *tsk,
> -                              unsigned long idx, u32 uctrl)
> +                              unsigned long idx, u64 addr,
> +                                  u64 mask, u32 uctrl)
>  {
>         int err;
>         struct perf_event *bp;
>         struct perf_event_attr attr;
> +       struct arch_hw_breakpoint *info;
>         struct arch_hw_breakpoint_ctrl ctrl;
>         struct thread_info *ti = task_thread_info(tsk);
>
> @@ -597,6 +599,17 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
>
>         attr = bp->attr;
>
> +       /* addr */
> +       /* Kernel-space address cannot be monitored by user-space */
> +       if ((unsigned long)addr >= XKPRANGE)
> +               return -EINVAL;
> +       attr.bp_addr = addr;
> +
> +       /* mask */
> +       info = counter_arch_bp(bp);
> +       info->mask = mask;
> +
> +       /* ctrl */
>         switch (note_type) {
>         case NT_LOONGARCH_HW_BREAK:
>                 ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
> @@ -623,46 +636,6 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
>         return modify_user_hw_breakpoint(bp, &attr);
>  }
>
> -static int ptrace_hbp_set_mask(unsigned int note_type,
> -                              struct task_struct *tsk,
> -                              unsigned long idx, u64 mask)
> -{
> -       struct perf_event *bp;
> -       struct perf_event_attr attr;
> -       struct arch_hw_breakpoint *info;
> -
> -       bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
> -       if (IS_ERR(bp))
> -               return PTR_ERR(bp);
> -
> -       attr = bp->attr;
> -       info = counter_arch_bp(bp);
> -       info->mask = mask;
> -
> -       return modify_user_hw_breakpoint(bp, &attr);
> -}
> -
> -static int ptrace_hbp_set_addr(unsigned int note_type,
> -                              struct task_struct *tsk,
> -                              unsigned long idx, u64 addr)
> -{
> -       struct perf_event *bp;
> -       struct perf_event_attr attr;
> -
> -       /* Kernel-space address cannot be monitored by user-space */
> -       if ((unsigned long)addr >= XKPRANGE)
> -               return -EINVAL;
> -
> -       bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
> -       if (IS_ERR(bp))
> -               return PTR_ERR(bp);
> -
> -       attr = bp->attr;
> -       attr.bp_addr = addr;
> -
> -       return modify_user_hw_breakpoint(bp, &attr);
> -}
> -
>  #define PTRACE_HBP_ADDR_SZ     sizeof(u64)
>  #define PTRACE_HBP_MASK_SZ     sizeof(u64)
>  #define PTRACE_HBP_CTRL_SZ     sizeof(u32)
> @@ -733,10 +706,6 @@ static int hw_break_set(struct task_struct *target,
>                                          offset, offset + PTRACE_HBP_ADDR_SZ);
>                 if (ret)
>                         return ret;
> -
> -               ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
> -               if (ret)
> -                       return ret;
>                 offset += PTRACE_HBP_ADDR_SZ;
>
>                 if (!count)
> @@ -746,21 +715,17 @@ static int hw_break_set(struct task_struct *target,
>                                          offset, offset + PTRACE_HBP_MASK_SZ);
>                 if (ret)
>                         return ret;
> -
> -               ret = ptrace_hbp_set_mask(note_type, target, idx, mask);
> -               if (ret)
> -                       return ret;
>                 offset += PTRACE_HBP_MASK_SZ;
>
>                 ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ctrl,
>                                          offset, offset + PTRACE_HBP_CTRL_SZ);
>                 if (ret)
>                         return ret;
> +               offset += PTRACE_HBP_CTRL_SZ;
>
> -               ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
> +               ret = ptrace_hbp_set(note_type, target, idx, addr, mask, ctrl);
>                 if (ret)
>                         return ret;
> -               offset += PTRACE_HBP_CTRL_SZ;
>
>                 user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
>                                           offset, offset + PTRACE_HBP_PAD_SZ);
> --
> 2.51.0
>

Re: [PATCH] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[Tiezhu Yang]

Hi Bill,

On 2025/11/5 上午8:57, Huacai Chen wrote:
> Hi, Tiezhu,
> 
> On Wed, Oct 29, 2025 at 11:38 PM Bill Tsui <b10902118@ntu.edu.tw> wrote:
>>
>> In hw_break_set(), those functions actually can be combined into one.
>> This eliminates intermediate calls to modify_user_hw_breakpoint() that
>> may leave hardware registers in a partially updated state.
>>
>> This redundancy was originally found in ARM ptrace, where it caused
>> non-4-byte address alignment checks to fail.
>>
>> Link:
>> https://lore.kernel.org/all/20251018133731.42505-2-b10902118@ntu.edu.tw/
>>
>> The LoongArch implementation appears to have been derived from ARM,
>> so this refactor helps avoid the same issue and simplifies future
>> maintenance.
> What do you think about it?

Can you describe directly what is wrong with the current code
on LoongArch? Please do not show the ARM's under-review patch
link in the commit message.

Does this patch fix real problem? If there is a test case to
reproduce the problem, please add it into the commit message.

If there is no functional change, just to refactor the code or
avoid the potential issues, please give a explicit description.
We will test GDB to see whether there is a regression.

Thanks,
Tiezhu

Re: [PATCH] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[Bill Tsui]

Hi Tiezhu,

> If there is no functional change, just to refactor the code or
> avoid the potential issues, please give a explicit description.
> We will test GDB to see whether there is a regression.

Yeah, just a refactor without functional change. I should have been
more explicit about the intent and that LoongArch doesn't share the
bug.

Let me resent the patch with clearer commit message.

Thanks,
Bill

[PATCH v2] LoongArch: ptrace: Merge ptrace_hbp_set_*()

[Bill Tsui]

In hw_break_set(), those functions actually can be combined into one.
This eliminates intermediate calls to modify_user_hw_breakpoint() that
may leave hardware registers in a partially updated state.

This is a refactor only; no functional change.

Signed-off-by: Bill Tsui <b10902118@ntu.edu.tw>
---
 arch/loongarch/kernel/ptrace.c | 69 +++++++++-------------------------
 1 file changed, 17 insertions(+), 52 deletions(-)

diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index 8edd0954e55a..c6cd51569a64 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -581,13 +581,15 @@ static int ptrace_hbp_get_addr(unsigned int note_type,
 	return 0;
 }
 
-static int ptrace_hbp_set_ctrl(unsigned int note_type,
+static int ptrace_hbp_set(unsigned int note_type,
 			       struct task_struct *tsk,
-			       unsigned long idx, u32 uctrl)
+			       unsigned long idx, u64 addr,
+				   u64 mask, u32 uctrl)
 {
 	int err;
 	struct perf_event *bp;
 	struct perf_event_attr attr;
+	struct arch_hw_breakpoint *info;
 	struct arch_hw_breakpoint_ctrl ctrl;
 	struct thread_info *ti = task_thread_info(tsk);
 
@@ -597,6 +599,17 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 
 	attr = bp->attr;
 
+	/* addr */
+	/* Kernel-space address cannot be monitored by user-space */
+	if ((unsigned long)addr >= XKPRANGE)
+		return -EINVAL;
+	attr.bp_addr = addr;
+
+	/* mask */
+	info = counter_arch_bp(bp);
+	info->mask = mask;
+
+	/* ctrl */
 	switch (note_type) {
 	case NT_LOONGARCH_HW_BREAK:
 		ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
@@ -623,46 +636,6 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 	return modify_user_hw_breakpoint(bp, &attr);
 }
 
-static int ptrace_hbp_set_mask(unsigned int note_type,
-			       struct task_struct *tsk,
-			       unsigned long idx, u64 mask)
-{
-	struct perf_event *bp;
-	struct perf_event_attr attr;
-	struct arch_hw_breakpoint *info;
-
-	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp))
-		return PTR_ERR(bp);
-
-	attr = bp->attr;
-	info = counter_arch_bp(bp);
-	info->mask = mask;
-
-	return modify_user_hw_breakpoint(bp, &attr);
-}
-
-static int ptrace_hbp_set_addr(unsigned int note_type,
-			       struct task_struct *tsk,
-			       unsigned long idx, u64 addr)
-{
-	struct perf_event *bp;
-	struct perf_event_attr attr;
-
-	/* Kernel-space address cannot be monitored by user-space */
-	if ((unsigned long)addr >= XKPRANGE)
-		return -EINVAL;
-
-	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp))
-		return PTR_ERR(bp);
-
-	attr = bp->attr;
-	attr.bp_addr = addr;
-
-	return modify_user_hw_breakpoint(bp, &attr);
-}
-
 #define PTRACE_HBP_ADDR_SZ	sizeof(u64)
 #define PTRACE_HBP_MASK_SZ	sizeof(u64)
 #define PTRACE_HBP_CTRL_SZ	sizeof(u32)
@@ -733,10 +706,6 @@ static int hw_break_set(struct task_struct *target,
 					 offset, offset + PTRACE_HBP_ADDR_SZ);
 		if (ret)
 			return ret;
-
-		ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
-		if (ret)
-			return ret;
 		offset += PTRACE_HBP_ADDR_SZ;
 
 		if (!count)
@@ -746,21 +715,17 @@ static int hw_break_set(struct task_struct *target,
 					 offset, offset + PTRACE_HBP_MASK_SZ);
 		if (ret)
 			return ret;
-
-		ret = ptrace_hbp_set_mask(note_type, target, idx, mask);
-		if (ret)
-			return ret;
 		offset += PTRACE_HBP_MASK_SZ;
 
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ctrl,
 					 offset, offset + PTRACE_HBP_CTRL_SZ);
 		if (ret)
 			return ret;
+		offset += PTRACE_HBP_CTRL_SZ;
 
-		ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
+		ret = ptrace_hbp_set(note_type, target, idx, addr, mask, ctrl);
 		if (ret)
 			return ret;
-		offset += PTRACE_HBP_CTRL_SZ;
 
 		user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
 					  offset, offset + PTRACE_HBP_PAD_SZ);
-- 
2.51.0