loongarch.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] LoongArch: Change the maximum number of watchpoints
@ 2025-01-21  7:06 Tiezhu Yang
  2025-01-21  7:06 ` [PATCH 1/2] LoongArch: Change 8 to 14 for LOONGARCH_MAX_{BRP,WRP} Tiezhu Yang
  2025-01-21  7:06 ` [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints Tiezhu Yang
  0 siblings, 2 replies; 7+ messages in thread
From: Tiezhu Yang @ 2025-01-21  7:06 UTC (permalink / raw)
  To: Huacai Chen; +Cc: loongarch, linux-kernel

Tiezhu Yang (2):
  LoongArch: Change 8 to 14 for LOONGARCH_MAX_{BRP,WRP}
  LoongArch: Extend the maximum number of watchpoints

 arch/loongarch/include/asm/hw_breakpoint.h |  4 +-
 arch/loongarch/include/asm/loongarch.h     | 60 ++++++++++++++++++++++
 arch/loongarch/include/uapi/asm/ptrace.h   | 10 ++++
 arch/loongarch/kernel/hw_breakpoint.c      | 16 +++++-
 arch/loongarch/kernel/ptrace.c             |  6 +--
 5 files changed, 89 insertions(+), 7 deletions(-)

-- 
2.42.0


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

* [PATCH 1/2] LoongArch: Change 8 to 14 for LOONGARCH_MAX_{BRP,WRP}
  2025-01-21  7:06 [PATCH 0/2] LoongArch: Change the maximum number of watchpoints Tiezhu Yang
@ 2025-01-21  7:06 ` Tiezhu Yang
  2025-01-21  7:06 ` [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints Tiezhu Yang
  1 sibling, 0 replies; 7+ messages in thread
From: Tiezhu Yang @ 2025-01-21  7:06 UTC (permalink / raw)
  To: Huacai Chen; +Cc: loongarch, linux-kernel

The maximum number of load/store watchpoints and fetch instruction
watchpoints is 14 each according to LoongArch Reference Manual, so
change 8 to 14 for the related code.

Link: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
Fixes: edffa33c7bb5 ("LoongArch: Add hardware breakpoints/watchpoints support")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/include/asm/hw_breakpoint.h |  4 +-
 arch/loongarch/include/asm/loongarch.h     | 60 ++++++++++++++++++++++
 arch/loongarch/kernel/hw_breakpoint.c      | 16 +++++-
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/include/asm/hw_breakpoint.h b/arch/loongarch/include/asm/hw_breakpoint.h
index d78330916bd1..13b2462f3d8c 100644
--- a/arch/loongarch/include/asm/hw_breakpoint.h
+++ b/arch/loongarch/include/asm/hw_breakpoint.h
@@ -38,8 +38,8 @@ struct arch_hw_breakpoint {
  * Limits.
  * Changing these will require modifications to the register accessors.
  */
-#define LOONGARCH_MAX_BRP		8
-#define LOONGARCH_MAX_WRP		8
+#define LOONGARCH_MAX_BRP		14
+#define LOONGARCH_MAX_WRP		14
 
 /* Virtual debug register bases. */
 #define CSR_CFG_ADDR	0
diff --git a/arch/loongarch/include/asm/loongarch.h b/arch/loongarch/include/asm/loongarch.h
index 64ad277e096e..aaa4ad6b8594 100644
--- a/arch/loongarch/include/asm/loongarch.h
+++ b/arch/loongarch/include/asm/loongarch.h
@@ -959,6 +959,36 @@
 #define LOONGARCH_CSR_DB7CTRL		0x34a	/* data breakpoint 7 control */
 #define LOONGARCH_CSR_DB7ASID		0x34b	/* data breakpoint 7 asid */
 
+#define LOONGARCH_CSR_DB8ADDR		0x350	/* data breakpoint 8 address */
+#define LOONGARCH_CSR_DB8MASK		0x351	/* data breakpoint 8 mask */
+#define LOONGARCH_CSR_DB8CTRL		0x352	/* data breakpoint 8 control */
+#define LOONGARCH_CSR_DB8ASID		0x353	/* data breakpoint 8 asid */
+
+#define LOONGARCH_CSR_DB9ADDR		0x358	/* data breakpoint 9 address */
+#define LOONGARCH_CSR_DB9MASK		0x359	/* data breakpoint 9 mask */
+#define LOONGARCH_CSR_DB9CTRL		0x35a	/* data breakpoint 9 control */
+#define LOONGARCH_CSR_DB9ASID		0x35b	/* data breakpoint 9 asid */
+
+#define LOONGARCH_CSR_DB10ADDR		0x360	/* data breakpoint 10 address */
+#define LOONGARCH_CSR_DB10MASK		0x361	/* data breakpoint 10 mask */
+#define LOONGARCH_CSR_DB10CTRL		0x362	/* data breakpoint 10 control */
+#define LOONGARCH_CSR_DB10ASID		0x363	/* data breakpoint 10 asid */
+
+#define LOONGARCH_CSR_DB11ADDR		0x368	/* data breakpoint 11 address */
+#define LOONGARCH_CSR_DB11MASK		0x369	/* data breakpoint 11 mask */
+#define LOONGARCH_CSR_DB11CTRL		0x36a	/* data breakpoint 11 control */
+#define LOONGARCH_CSR_DB11ASID		0x36b	/* data breakpoint 11 asid */
+
+#define LOONGARCH_CSR_DB12ADDR		0x370	/* data breakpoint 12 address */
+#define LOONGARCH_CSR_DB12MASK		0x371	/* data breakpoint 12 mask */
+#define LOONGARCH_CSR_DB12CTRL		0x372	/* data breakpoint 12 control */
+#define LOONGARCH_CSR_DB12ASID		0x373	/* data breakpoint 12 asid */
+
+#define LOONGARCH_CSR_DB13ADDR		0x378	/* data breakpoint 13 address */
+#define LOONGARCH_CSR_DB13MASK		0x379	/* data breakpoint 13 mask */
+#define LOONGARCH_CSR_DB13CTRL		0x37a	/* data breakpoint 13 control */
+#define LOONGARCH_CSR_DB13ASID		0x37b	/* data breakpoint 13 asid */
+
 #define LOONGARCH_CSR_FWPC		0x380	/* instruction breakpoint config */
 #define LOONGARCH_CSR_FWPS		0x381	/* instruction breakpoint status */
 
@@ -1002,6 +1032,36 @@
 #define LOONGARCH_CSR_IB7CTRL		0x3ca	/* inst breakpoint 7 control */
 #define LOONGARCH_CSR_IB7ASID		0x3cb	/* inst breakpoint 7 asid */
 
+#define LOONGARCH_CSR_IB8ADDR		0x3d0	/* inst breakpoint 8 address */
+#define LOONGARCH_CSR_IB8MASK		0x3d1	/* inst breakpoint 8 mask */
+#define LOONGARCH_CSR_IB8CTRL		0x3d2	/* inst breakpoint 8 control */
+#define LOONGARCH_CSR_IB8ASID		0x3d3	/* inst breakpoint 8 asid */
+
+#define LOONGARCH_CSR_IB9ADDR		0x3d8	/* inst breakpoint 9 address */
+#define LOONGARCH_CSR_IB9MASK		0x3d9	/* inst breakpoint 9 mask */
+#define LOONGARCH_CSR_IB9CTRL		0x3da	/* inst breakpoint 9 control */
+#define LOONGARCH_CSR_IB9ASID		0x3db	/* inst breakpoint 9 asid */
+
+#define LOONGARCH_CSR_IB10ADDR		0x3e0	/* inst breakpoint 10 address */
+#define LOONGARCH_CSR_IB10MASK		0x3e1	/* inst breakpoint 10 mask */
+#define LOONGARCH_CSR_IB10CTRL		0x3e2	/* inst breakpoint 10 control */
+#define LOONGARCH_CSR_IB10ASID		0x3e3	/* inst breakpoint 10 asid */
+
+#define LOONGARCH_CSR_IB11ADDR		0x3e8	/* inst breakpoint 11 address */
+#define LOONGARCH_CSR_IB11MASK		0x3e9	/* inst breakpoint 11 mask */
+#define LOONGARCH_CSR_IB11CTRL		0x3ea	/* inst breakpoint 11 control */
+#define LOONGARCH_CSR_IB11ASID		0x3eb	/* inst breakpoint 11 asid */
+
+#define LOONGARCH_CSR_IB12ADDR		0x3f0	/* inst breakpoint 12 address */
+#define LOONGARCH_CSR_IB12MASK		0x3f1	/* inst breakpoint 12 mask */
+#define LOONGARCH_CSR_IB12CTRL		0x3f2	/* inst breakpoint 12 control */
+#define LOONGARCH_CSR_IB12ASID		0x3f3	/* inst breakpoint 12 asid */
+
+#define LOONGARCH_CSR_IB13ADDR		0x3f8	/* inst breakpoint 13 address */
+#define LOONGARCH_CSR_IB13MASK		0x3f9	/* inst breakpoint 13 mask */
+#define LOONGARCH_CSR_IB13CTRL		0x3fa	/* inst breakpoint 13 control */
+#define LOONGARCH_CSR_IB13ASID		0x3fb	/* inst breakpoint 13 asid */
+
 #define LOONGARCH_CSR_DEBUG		0x500	/* debug config */
 #define LOONGARCH_CSR_DERA		0x501	/* debug era */
 #define LOONGARCH_CSR_DESAVE		0x502	/* debug save */
diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
index a6e4b605bfa8..c35f9bf38033 100644
--- a/arch/loongarch/kernel/hw_breakpoint.c
+++ b/arch/loongarch/kernel/hw_breakpoint.c
@@ -51,7 +51,13 @@ int hw_breakpoint_slots(int type)
 	READ_WB_REG_CASE(OFF, 4, REG, T, VAL);		\
 	READ_WB_REG_CASE(OFF, 5, REG, T, VAL);		\
 	READ_WB_REG_CASE(OFF, 6, REG, T, VAL);		\
-	READ_WB_REG_CASE(OFF, 7, REG, T, VAL);
+	READ_WB_REG_CASE(OFF, 7, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 8, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 9, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 10, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 11, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 12, REG, T, VAL);		\
+	READ_WB_REG_CASE(OFF, 13, REG, T, VAL);
 
 #define GEN_WRITE_WB_REG_CASES(OFF, REG, T, VAL)	\
 	WRITE_WB_REG_CASE(OFF, 0, REG, T, VAL);		\
@@ -61,7 +67,13 @@ int hw_breakpoint_slots(int type)
 	WRITE_WB_REG_CASE(OFF, 4, REG, T, VAL);		\
 	WRITE_WB_REG_CASE(OFF, 5, REG, T, VAL);		\
 	WRITE_WB_REG_CASE(OFF, 6, REG, T, VAL);		\
-	WRITE_WB_REG_CASE(OFF, 7, REG, T, VAL);
+	WRITE_WB_REG_CASE(OFF, 7, REG, T, VAL);		\
+	WRITE_WB_REG_CASE(OFF, 8, REG, T, VAL);		\
+	WRITE_WB_REG_CASE(OFF, 9, REG, T, VAL);		\
+	WRITE_WB_REG_CASE(OFF, 10, REG, T, VAL);	\
+	WRITE_WB_REG_CASE(OFF, 11, REG, T, VAL);	\
+	WRITE_WB_REG_CASE(OFF, 12, REG, T, VAL);	\
+	WRITE_WB_REG_CASE(OFF, 13, REG, T, VAL);
 
 static u64 read_wb_reg(int reg, int n, int t)
 {
-- 
2.42.0


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

* [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints
  2025-01-21  7:06 [PATCH 0/2] LoongArch: Change the maximum number of watchpoints Tiezhu Yang
  2025-01-21  7:06 ` [PATCH 1/2] LoongArch: Change 8 to 14 for LOONGARCH_MAX_{BRP,WRP} Tiezhu Yang
@ 2025-01-21  7:06 ` Tiezhu Yang
  2025-01-21 13:35   ` WANG Xuerui
  1 sibling, 1 reply; 7+ messages in thread
From: Tiezhu Yang @ 2025-01-21  7:06 UTC (permalink / raw)
  To: Huacai Chen; +Cc: loongarch, linux-kernel

The maximum number of load/store watchpoints and fetch instruction
watchpoints is 14 each according to LoongArch Reference Manual, so
extend the maximum number of watchpoints from 8 to 14 for ptrace.

By the way, just simply change 8 to 14 for the definition in struct
user_watch_state at the beginning, but it may corrupt uapi, then add
a new struct user_watch_state_v2 directly.

As far as I can tell, the only users for this struct in the userspace
are GDB and LLDB, there are no any problems of software compatibility
between the application and kernel according to the analysis.

Link: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
Fixes: 1a69f7a161a7 ("LoongArch: ptrace: Expose hardware breakpoints to debuggers")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/include/uapi/asm/ptrace.h | 10 ++++++++++
 arch/loongarch/kernel/ptrace.c           |  6 +++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/loongarch/include/uapi/asm/ptrace.h b/arch/loongarch/include/uapi/asm/ptrace.h
index ac915f841650..aafb3cd9e943 100644
--- a/arch/loongarch/include/uapi/asm/ptrace.h
+++ b/arch/loongarch/include/uapi/asm/ptrace.h
@@ -72,6 +72,16 @@ struct user_watch_state {
 	} dbg_regs[8];
 };
 
+struct user_watch_state_v2 {
+	uint64_t dbg_info;
+	struct {
+		uint64_t    addr;
+		uint64_t    mask;
+		uint32_t    ctrl;
+		uint32_t    pad;
+	} dbg_regs[14];
+};
+
 #define PTRACE_SYSEMU			0x1f
 #define PTRACE_SYSEMU_SINGLESTEP	0x20
 
diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index 19dc6eff45cc..5e2402cfcab0 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -720,7 +720,7 @@ static int hw_break_set(struct task_struct *target,
 	unsigned int note_type = regset->core_note_type;
 
 	/* Resource info */
-	offset = offsetof(struct user_watch_state, dbg_regs);
+	offset = offsetof(struct user_watch_state_v2, dbg_regs);
 	user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
 
 	/* (address, mask, ctrl) registers */
@@ -920,7 +920,7 @@ static const struct user_regset loongarch64_regsets[] = {
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	[REGSET_HW_BREAK] = {
 		.core_note_type = NT_LOONGARCH_HW_BREAK,
-		.n = sizeof(struct user_watch_state) / sizeof(u32),
+		.n = sizeof(struct user_watch_state_v2) / sizeof(u32),
 		.size = sizeof(u32),
 		.align = sizeof(u32),
 		.regset_get = hw_break_get,
@@ -928,7 +928,7 @@ static const struct user_regset loongarch64_regsets[] = {
 	},
 	[REGSET_HW_WATCH] = {
 		.core_note_type = NT_LOONGARCH_HW_WATCH,
-		.n = sizeof(struct user_watch_state) / sizeof(u32),
+		.n = sizeof(struct user_watch_state_v2) / sizeof(u32),
 		.size = sizeof(u32),
 		.align = sizeof(u32),
 		.regset_get = hw_break_get,
-- 
2.42.0


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

* Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints
  2025-01-21  7:06 ` [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints Tiezhu Yang
@ 2025-01-21 13:35   ` WANG Xuerui
  2025-01-21 13:46     ` Xi Ruoyao
  0 siblings, 1 reply; 7+ messages in thread
From: WANG Xuerui @ 2025-01-21 13:35 UTC (permalink / raw)
  To: Tiezhu Yang, Huacai Chen; +Cc: loongarch, linux-kernel

Hi,

On 1/21/25 15:06, Tiezhu Yang wrote:
> The maximum number of load/store watchpoints and fetch instruction
> watchpoints is 14 each according to LoongArch Reference Manual, so
> extend the maximum number of watchpoints from 8 to 14 for ptrace.
> 
> By the way, just simply change 8 to 14 for the definition in struct
> user_watch_state at the beginning, but it may corrupt uapi, then add
> a new struct user_watch_state_v2 directly.
> 
> As far as I can tell, the only users for this struct in the userspace
> are GDB and LLDB, there are no any problems of software compatibility
> between the application and kernel according to the analysis.

But it seems something is indeed corrupted by this patch...

For example, it's entirely appropriate to mix-match debuggers and 
kernels of different UAPI revision. In this case, some kind of version 
marker must be present in the UAPI structure to prevent out-of-bounds 
accesses by the kernel. But according to the changes...

> Link: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
> Fixes: 1a69f7a161a7 ("LoongArch: ptrace: Expose hardware breakpoints to debuggers")
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/loongarch/include/uapi/asm/ptrace.h | 10 ++++++++++
>   arch/loongarch/kernel/ptrace.c           |  6 +++---
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h b/arch/loongarch/include/uapi/asm/ptrace.h
> index ac915f841650..aafb3cd9e943 100644
> --- a/arch/loongarch/include/uapi/asm/ptrace.h
> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
> @@ -72,6 +72,16 @@ struct user_watch_state {
>   	} dbg_regs[8];
>   };
>   
> +struct user_watch_state_v2 {
> +	uint64_t dbg_info;
> +	struct {
> +		uint64_t    addr;
> +		uint64_t    mask;
> +		uint32_t    ctrl;
> +		uint32_t    pad;
> +	} dbg_regs[14];
> +};

there seems no indication of any "version marker" field ("revision" or 
"sizeof(input)" or something like that), so...

> +
>   #define PTRACE_SYSEMU			0x1f
>   #define PTRACE_SYSEMU_SINGLESTEP	0x20
>   
> diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
> index 19dc6eff45cc..5e2402cfcab0 100644
> --- a/arch/loongarch/kernel/ptrace.c
> +++ b/arch/loongarch/kernel/ptrace.c
> @@ -720,7 +720,7 @@ static int hw_break_set(struct task_struct *target,
>   	unsigned int note_type = regset->core_note_type;
>   
>   	/* Resource info */
> -	offset = offsetof(struct user_watch_state, dbg_regs);
> +	offset = offsetof(struct user_watch_state_v2, dbg_regs);
>   	user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
>   
>   	/* (address, mask, ctrl) registers */
> @@ -920,7 +920,7 @@ static const struct user_regset loongarch64_regsets[] = {
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>   	[REGSET_HW_BREAK] = {
>   		.core_note_type = NT_LOONGARCH_HW_BREAK,
> -		.n = sizeof(struct user_watch_state) / sizeof(u32),
> +		.n = sizeof(struct user_watch_state_v2) / sizeof(u32),

one cannot account for older (in fact, *every existing*) clients 
expecting the old UAPI, hence providing a smaller buffer than struct 
user_watch_state_v2. In which case the kernel will happily write 
out-of-bounds if the client asks for the current watchpoint state. This 
is not acceptable I'm afraid.

>   		.size = sizeof(u32),
>   		.align = sizeof(u32),
>   		.regset_get = hw_break_get,
> @@ -928,7 +928,7 @@ static const struct user_regset loongarch64_regsets[] = {
>   	},
>   	[REGSET_HW_WATCH] = {
>   		.core_note_type = NT_LOONGARCH_HW_WATCH,
> -		.n = sizeof(struct user_watch_state) / sizeof(u32),
> +		.n = sizeof(struct user_watch_state_v2) / sizeof(u32),
>   		.size = sizeof(u32),
>   		.align = sizeof(u32),
>   		.regset_get = hw_break_get,


-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

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

* Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints
  2025-01-21 13:35   ` WANG Xuerui
@ 2025-01-21 13:46     ` Xi Ruoyao
  2025-01-22  3:09       ` Tiezhu Yang
  0 siblings, 1 reply; 7+ messages in thread
From: Xi Ruoyao @ 2025-01-21 13:46 UTC (permalink / raw)
  To: WANG Xuerui, Tiezhu Yang, Huacai Chen; +Cc: loongarch, linux-kernel

On Tue, 2025-01-21 at 21:35 +0800, WANG Xuerui wrote:
> one cannot account for older (in fact, *every existing*) clients 
> expecting the old UAPI, hence providing a smaller buffer than struct 
> user_watch_state_v2. In which case the kernel will happily write 
> out-of-bounds if the client asks for the current watchpoint state. This 
> is not acceptable I'm afraid.

Yes, many distros support running the latest kernel with an old
userspace for hardware compatibility reason and the change will blow up
gdb on all those distros.  We need to do something smarter to retain the
backward compatibility.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

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

* Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints
  2025-01-21 13:46     ` Xi Ruoyao
@ 2025-01-22  3:09       ` Tiezhu Yang
  2025-01-24 16:09         ` WANG Xuerui
  0 siblings, 1 reply; 7+ messages in thread
From: Tiezhu Yang @ 2025-01-22  3:09 UTC (permalink / raw)
  To: Xi Ruoyao, WANG Xuerui, Huacai Chen; +Cc: loongarch, linux-kernel

On 01/21/2025 09:46 PM, Xi Ruoyao wrote:
> On Tue, 2025-01-21 at 21:35 +0800, WANG Xuerui wrote:
>> one cannot account for older (in fact, *every existing*) clients
>> expecting the old UAPI, hence providing a smaller buffer than struct
>> user_watch_state_v2. In which case the kernel will happily write
>> out-of-bounds if the client asks for the current watchpoint state. This
>> is not acceptable I'm afraid.
>
> Yes, many distros support running the latest kernel with an old
> userspace for hardware compatibility reason and the change will blow up
> gdb on all those distros.  We need to do something smarter to retain the
> backward compatibility.

Hi Xuerui and Ruoyao,

The compatibility problem has been considered when developing and testing
the patches, when the applications in the userspace get watchpoint state,
the length will be specified which will not bigger than the sizeof struct
user_watch_state or user_watch_state_v2, the actual length is assigned as
the minimal value of the application and kernel in the generic ptrace:

kernel/ptrace.c: ptrace_regset():

	kiov->iov_len = min(kiov->iov_len,
			            (__kernel_size_t) (regset->n * regset->size));

	if (req == PTRACE_GETREGSET)
		    return copy_regset_to_user(task, view, regset_no, 0,
					                   kiov->iov_len, kiov->iov_base);
	else
		    return copy_regset_from_user(task, view, regset_no, 0,
					                     kiov->iov_len, kiov->iov_base);

For example, there are four kind of combinations, all of them work well.

(1) "older kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200;
(2) "newer kernel + newer gdb", the actual length is 8+(8+8+4+4)*14=344;
(3) "older kernel + newer gdb", the actual length is 8+(8+8+4+4)*8=200;
(4) "newer kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200.

If you have more comments, please let me know.


Hi Huacai,

Could you please update the commit message when applying the patches
if you think the changes are OK? Like this:

----------------------
The maximum number of load/store watchpoints and fetch instruction
watchpoints is 14 each according to LoongArch Reference Manual, so
extend the maximum number of watchpoints from 8 to 14 for ptrace.

By the way, just simply change 8 to 14 for the definition in struct
user_watch_state at the beginning, but it may corrupt uapi, then add
a new struct user_watch_state_v2 directly.

As far as I can tell, the only users for this struct in the userspace
are GDB and LLDB, there are no any problems of software compatibility
between the application and kernel according to the analysis.

The compatibility problem has been considered when developing and testing
the patches, when the applications in the userspace get watchpoint state,
the length will be specified which will not bigger than the sizeof struct
user_watch_state or user_watch_state_v2, the actual length is assigned as
the minimal value of the application and kernel in the generic ptrace:

kernel/ptrace.c: ptrace_regset():

	kiov->iov_len = min(kiov->iov_len,
			            (__kernel_size_t) (regset->n * regset->size));

	if (req == PTRACE_GETREGSET)
		    return copy_regset_to_user(task, view, regset_no, 0,
					                   kiov->iov_len, kiov->iov_base);
	else
		    return copy_regset_from_user(task, view, regset_no, 0,
					                     kiov->iov_len, kiov->iov_base);

For example, there are four kind of combinations, all of them work well.

(1) "older kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200;
(2) "newer kernel + newer gdb", the actual length is 8+(8+8+4+4)*14=344;
(3) "older kernel + newer gdb", the actual length is 8+(8+8+4+4)*8=200;
(4) "newer kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200.
----------------------

Thanks,
Tiezhu


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

* Re: [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints
  2025-01-22  3:09       ` Tiezhu Yang
@ 2025-01-24 16:09         ` WANG Xuerui
  0 siblings, 0 replies; 7+ messages in thread
From: WANG Xuerui @ 2025-01-24 16:09 UTC (permalink / raw)
  To: Tiezhu Yang, Xi Ruoyao, Huacai Chen; +Cc: loongarch, linux-kernel

On 1/22/25 11:09, Tiezhu Yang wrote:
> On 01/21/2025 09:46 PM, Xi Ruoyao wrote:
>> On Tue, 2025-01-21 at 21:35 +0800, WANG Xuerui wrote:
>>> one cannot account for older (in fact, *every existing*) clients
>>> expecting the old UAPI, hence providing a smaller buffer than struct
>>> user_watch_state_v2. In which case the kernel will happily write
>>> out-of-bounds if the client asks for the current watchpoint state. This
>>> is not acceptable I'm afraid.
>>
>> Yes, many distros support running the latest kernel with an old
>> userspace for hardware compatibility reason and the change will blow up
>> gdb on all those distros.  We need to do something smarter to retain the
>> backward compatibility.
> 
> Hi Xuerui and Ruoyao,
> 
> The compatibility problem has been considered when developing and testing
> the patches, when the applications in the userspace get watchpoint state,
> the length will be specified which will not bigger than the sizeof struct
> user_watch_state or user_watch_state_v2, the actual length is assigned as
> the minimal value of the application and kernel in the generic ptrace:
> 
> kernel/ptrace.c: ptrace_regset():
> 
>      kiov->iov_len = min(kiov->iov_len,
>                          (__kernel_size_t) (regset->n * regset->size));
> 
>      if (req == PTRACE_GETREGSET)
>              return copy_regset_to_user(task, view, regset_no, 0,
>                                         kiov->iov_len, kiov->iov_base);
>      else
>              return copy_regset_from_user(task, view, regset_no, 0,
>                                           kiov->iov_len, kiov->iov_base);
> 
> For example, there are four kind of combinations, all of them work well.
> 
> (1) "older kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200;
> (2) "newer kernel + newer gdb", the actual length is 8+(8+8+4+4)*14=344;
> (3) "older kernel + newer gdb", the actual length is 8+(8+8+4+4)*8=200;
> (4) "newer kernel + older gdb", the actual length is 8+(8+8+4+4)*8=200.
> 
> If you have more comments, please let me know.

Thanks for posting the analysis; it was good to know the equivalent of 
sizeof(input) handling actually resides in the arch-independent side.

But next time please just include the explanation in v1 commit message 
to save us many replies back-and-forth. ;-)

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

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

end of thread, other threads:[~2025-01-24 16:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-21  7:06 [PATCH 0/2] LoongArch: Change the maximum number of watchpoints Tiezhu Yang
2025-01-21  7:06 ` [PATCH 1/2] LoongArch: Change 8 to 14 for LOONGARCH_MAX_{BRP,WRP} Tiezhu Yang
2025-01-21  7:06 ` [PATCH 2/2] LoongArch: Extend the maximum number of watchpoints Tiezhu Yang
2025-01-21 13:35   ` WANG Xuerui
2025-01-21 13:46     ` Xi Ruoyao
2025-01-22  3:09       ` Tiezhu Yang
2025-01-24 16:09         ` WANG Xuerui

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).