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