This is done in order to easily calculate the number of breakpoints
in hw_break_get.
Signed-off-by: Qing Zhang <[email protected]>
---
arch/loongarch/include/uapi/asm/ptrace.h | 3 ++-
arch/loongarch/kernel/ptrace.c | 13 +++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/loongarch/include/uapi/asm/ptrace.h b/arch/loongarch/include/uapi/asm/ptrace.h
index 2282ae1fd3b6..06e3be52cb04 100644
--- a/arch/loongarch/include/uapi/asm/ptrace.h
+++ b/arch/loongarch/include/uapi/asm/ptrace.h
@@ -57,11 +57,12 @@ struct user_lasx_state {
};
struct user_watch_state {
- uint16_t dbg_info;
+ uint64_t dbg_info;
struct {
uint64_t addr;
uint64_t mask;
uint32_t ctrl;
+ uint32_t pad;
} dbg_regs[8];
};
diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index 0c7c41e41cad..9c3bc1bbf2ff 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
return 0;
}
-static int ptrace_hbp_get_resource_info(unsigned int note_type, u16 *info)
+static int ptrace_hbp_get_resource_info(unsigned int note_type, u64 *info)
{
u8 num;
- u16 reg = 0;
+ u64 reg = 0;
switch (note_type) {
case NT_LOONGARCH_HW_BREAK:
@@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct *target,
const struct user_regset *regset,
struct membuf to)
{
- u16 info;
+ u64 info;
u32 ctrl;
u64 addr, mask;
int ret, idx = 0;
@@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct *target,
membuf_store(&to, addr);
membuf_store(&to, mask);
membuf_store(&to, ctrl);
+ membuf_zero(&to, sizeof(u32));
idx++;
}
@@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct *target,
int ret, idx = 0, offset, limit;
unsigned int note_type = regset->core_note_type;
- /* Resource info */
+ /* Resource info and pad */
offset = offsetof(struct user_watch_state, dbg_regs);
user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
@@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct *target,
if (ret)
return ret;
offset += PTRACE_HBP_CTRL_SZ;
+
+ user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
+ offset, offset + PTRACE_HBP_PAD_SZ);
+ offset += PTRACE_HBP_PAD_SZ;
idx++;
}
--
2.20.1
Hi Qing,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.3-rc7 next-20230417]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Qing-Zhang/LoongArch-Adjust-the-user_regset_copyin-parameter-to-the-correct-offset/20230418-171556
patch link: https://lore.kernel.org/r/20230418091348.9239-1-zhangqing%40loongson.cn
patch subject: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment
config: loongarch-defconfig (https://download.01.org/0day-ci/archive/20230419/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/dace28025b7b1f35b35042ddac8bdb1e412c2d7f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Qing-Zhang/LoongArch-Adjust-the-user_regset_copyin-parameter-to-the-correct-offset/20230418-171556
git checkout dace28025b7b1f35b35042ddac8bdb1e412c2d7f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
arch/loongarch/kernel/ptrace.c: In function 'hw_break_set':
>> arch/loongarch/kernel/ptrace.c:626:60: error: 'PTRACE_HBP_PAD_SZ' undeclared (first use in this function); did you mean 'PTRACE_HBP_MASK_SZ'?
626 | offset, offset + PTRACE_HBP_PAD_SZ);
| ^~~~~~~~~~~~~~~~~
| PTRACE_HBP_MASK_SZ
arch/loongarch/kernel/ptrace.c:626:60: note: each undeclared identifier is reported only once for each function it appears in
vim +626 arch/loongarch/kernel/ptrace.c
571
572 static int hw_break_set(struct task_struct *target,
573 const struct user_regset *regset,
574 unsigned int pos, unsigned int count,
575 const void *kbuf, const void __user *ubuf)
576 {
577 u32 ctrl;
578 u64 addr, mask;
579 int ret, idx = 0, offset, limit;
580 unsigned int note_type = regset->core_note_type;
581
582 /* Resource info and pad */
583 offset = offsetof(struct user_watch_state, dbg_regs);
584 user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
585
586 /* (address, ctrl) registers */
587 limit = regset->n * regset->size;
588 while (count && offset < limit) {
589 if (count < PTRACE_HBP_ADDR_SZ)
590 return -EINVAL;
591
592 ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
593 offset, offset + PTRACE_HBP_ADDR_SZ);
594 if (ret)
595 return ret;
596
597 ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
598 if (ret)
599 return ret;
600 offset += PTRACE_HBP_ADDR_SZ;
601
602 if (!count)
603 break;
604
605 ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &mask,
606 offset, offset + PTRACE_HBP_ADDR_SZ);
607 if (ret)
608 return ret;
609
610 ret = ptrace_hbp_set_mask(note_type, target, idx, mask);
611 if (ret)
612 return ret;
613 offset += PTRACE_HBP_MASK_SZ;
614
615 ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &mask,
616 offset, offset + PTRACE_HBP_MASK_SZ);
617 if (ret)
618 return ret;
619
620 ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
621 if (ret)
622 return ret;
623 offset += PTRACE_HBP_CTRL_SZ;
624
625 user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> 626 offset, offset + PTRACE_HBP_PAD_SZ);
627 offset += PTRACE_HBP_PAD_SZ;
628 idx++;
629 }
630
631 return 0;
632 }
633
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
> This is done in order to easily calculate the number of breakpoints
> in hw_break_get.
>
> Signed-off-by: Qing Zhang <[email protected]>
> ---
> arch/loongarch/include/uapi/asm/ptrace.h | 3 ++-
> arch/loongarch/kernel/ptrace.c | 13 +++++++++----
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
> b/arch/loongarch/include/uapi/asm/ptrace.h
> index 2282ae1fd3b6..06e3be52cb04 100644
> --- a/arch/loongarch/include/uapi/asm/ptrace.h
> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
> @@ -57,11 +57,12 @@ struct user_lasx_state {
> };
>
> struct user_watch_state {
> - uint16_t dbg_info;
> + uint64_t dbg_info;
Ouch. This is a breaking change when we consider user code like
`printf(PRIu16 "\n", ptr->dbg_info);`. Is it really necessary?
> struct {
> uint64_t addr;
> uint64_t mask;
> uint32_t ctrl;
> + uint32_t pad;
> } dbg_regs[8];
> };
>
> diff --git a/arch/loongarch/kernel/ptrace.c
> b/arch/loongarch/kernel/ptrace.c
> index 0c7c41e41cad..9c3bc1bbf2ff 100644
> --- a/arch/loongarch/kernel/ptrace.c
> +++ b/arch/loongarch/kernel/ptrace.c
> @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
> int note_type,
> return 0;
> }
>
> -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
> *info)
> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
> *info)
> {
> u8 num;
> - u16 reg = 0;
> + u64 reg = 0;
>
> switch (note_type) {
> case NT_LOONGARCH_HW_BREAK:
> @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
> *target,
> const struct user_regset *regset,
> struct membuf to)
> {
> - u16 info;
> + u64 info;
> u32 ctrl;
> u64 addr, mask;
> int ret, idx = 0;
> @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
> *target,
> membuf_store(&to, addr);
> membuf_store(&to, mask);
> membuf_store(&to, ctrl);
> + membuf_zero(&to, sizeof(u32));
> idx++;
> }
>
> @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
> *target,
> int ret, idx = 0, offset, limit;
> unsigned int note_type = regset->core_note_type;
>
> - /* Resource info */
> + /* Resource info and pad */
> offset = offsetof(struct user_watch_state, dbg_regs);
> user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
> offset);
>
> @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
> *target,
> if (ret)
> return ret;
> offset += PTRACE_HBP_CTRL_SZ;
> +
> + user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> + offset, offset +
> PTRACE_HBP_PAD_SZ);
> + offset += PTRACE_HBP_PAD_SZ;
> idx++;
> }
>
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On 2023/4/19 18:42, Xi Ruoyao wrote:
> On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
>> This is done in order to easily calculate the number of breakpoints
>> in hw_break_get.
>>
>> Signed-off-by: Qing Zhang <[email protected]>
>> ---
>> arch/loongarch/include/uapi/asm/ptrace.h | 3 ++-
>> arch/loongarch/kernel/ptrace.c | 13 +++++++++----
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
>> b/arch/loongarch/include/uapi/asm/ptrace.h
>> index 2282ae1fd3b6..06e3be52cb04 100644
>> --- a/arch/loongarch/include/uapi/asm/ptrace.h
>> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
>> @@ -57,11 +57,12 @@ struct user_lasx_state {
Drive-by comment to the patch author: there is no "user_lasx_state" yet.
Please always state your base commit if not obvious, or you should start
from some well-known upstream HEAD (e.g. Linus' rc tags,
loongarch-fixes, or loongarch-next).
>> };
>>
>> struct user_watch_state {
>> - uint16_t dbg_info;
>> + uint64_t dbg_info;
>
> Ouch. This is a breaking change when we consider user code like
> `printf(PRIu16 "\n", ptr->dbg_info);`. Is it really necessary?
Ah right. This is UAPI so without *very* concrete and convicing reason
why the change is not going to impact any potential users, it's gonna be
a presumed NAK. In other words you must demonstrate (1) why it's
absolutely necessary to make the change and (2) that it's impossible to
impact anyone, before any such changes can even be considered.
>
>> struct {
>> uint64_t addr;
>> uint64_t mask;
>> uint32_t ctrl;
>> + uint32_t pad;
>> } dbg_regs[8];
>> };
>>
>> diff --git a/arch/loongarch/kernel/ptrace.c
>> b/arch/loongarch/kernel/ptrace.c
>> index 0c7c41e41cad..9c3bc1bbf2ff 100644
>> --- a/arch/loongarch/kernel/ptrace.c
>> +++ b/arch/loongarch/kernel/ptrace.c
>> @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
>> int note_type,
>> return 0;
>> }
>>
>> -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
>> *info)
>> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
>> *info)
>> {
>> u8 num;
>> - u16 reg = 0;
>> + u64 reg = 0;
>>
>> switch (note_type) {
>> case NT_LOONGARCH_HW_BREAK:
>> @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
>> *target,
>> const struct user_regset *regset,
>> struct membuf to)
>> {
>> - u16 info;
>> + u64 info;
>> u32 ctrl;
>> u64 addr, mask;
>> int ret, idx = 0;
>> @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
>> *target,
>> membuf_store(&to, addr);
>> membuf_store(&to, mask);
>> membuf_store(&to, ctrl);
>> + membuf_zero(&to, sizeof(u32));
>> idx++;
>> }
>>
>> @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
>> *target,
>> int ret, idx = 0, offset, limit;
>> unsigned int note_type = regset->core_note_type;
>>
>> - /* Resource info */
>> + /* Resource info and pad */
>> offset = offsetof(struct user_watch_state, dbg_regs);
>> user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
>> offset);
>>
>> @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
>> *target,
>> if (ret)
>> return ret;
>> offset += PTRACE_HBP_CTRL_SZ;
>> +
>> + user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
>> + offset, offset +
>> PTRACE_HBP_PAD_SZ);
>> + offset += PTRACE_HBP_PAD_SZ;
>> idx++;
>> }
>>
>
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
On 4/19/23 19:00, WANG Xuerui wrote:
> On 2023/4/19 18:42, Xi Ruoyao wrote:
>> On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
>>> This is done in order to easily calculate the number of breakpoints
>>> in hw_break_get.
>>>
>>> Signed-off-by: Qing Zhang <[email protected]>
>>> ---
>>> arch/loongarch/include/uapi/asm/ptrace.h | 3 ++-
>>> arch/loongarch/kernel/ptrace.c | 13 +++++++++----
>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
>>> b/arch/loongarch/include/uapi/asm/ptrace.h
>>> index 2282ae1fd3b6..06e3be52cb04 100644
>>> --- a/arch/loongarch/include/uapi/asm/ptrace.h
>>> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
>>> @@ -57,11 +57,12 @@ struct user_lasx_state {
>
> Drive-by comment to the patch author: there is no "user_lasx_state"
> yet. Please always state your base commit if not obvious, or you
> should start from some well-known upstream HEAD (e.g. Linus' rc tags,
> loongarch-fixes, or loongarch-next).
>
>>> };
>>> struct user_watch_state {
>>> - uint16_t dbg_info;
>>> + uint64_t dbg_info;
>>
>> Ouch. This is a breaking change when we consider user code like
>> `printf(PRIu16 "\n", ptr->dbg_info);`. Is it really necessary?
>
> Ah right. This is UAPI so without *very* concrete and convicing reason
> why the change is not going to impact any potential users, it's gonna
> be a presumed NAK. In other words you must demonstrate (1) why it's
> absolutely necessary to make the change and (2) that it's impossible
> to impact anyone, before any such changes can even be considered.
Please ignore all of this. The memory layout is actually the same after
the change due to the padding, I was somehow thinking in big-endian a
few hours ago. (The commit message didn't help either, I think both
Ruoyao and me got into the habitual thinking that changes like this are
most likely just churn without real benefits, after *not* seeing the
rationale in the commit message which was kinda expected.)
>
>>
>>> struct {
>>> uint64_t addr;
>>> uint64_t mask;
>>> uint32_t ctrl;
>>> + uint32_t pad;
>>> } dbg_regs[8];
>>> };
>>> diff --git a/arch/loongarch/kernel/ptrace.c
>>> b/arch/loongarch/kernel/ptrace.c
>>> index 0c7c41e41cad..9c3bc1bbf2ff 100644
>>> --- a/arch/loongarch/kernel/ptrace.c
>>> +++ b/arch/loongarch/kernel/ptrace.c
>>> @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
>>> int note_type,
>>> return 0;
>>> }
>>> -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
>>> *info)
>>> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
>>> *info)
>>> {
>>> u8 num;
>>> - u16 reg = 0;
>>> + u64 reg = 0;
>>> switch (note_type) {
>>> case NT_LOONGARCH_HW_BREAK:
>>> @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
>>> *target,
>>> const struct user_regset *regset,
>>> struct membuf to)
>>> {
>>> - u16 info;
>>> + u64 info;
>>> u32 ctrl;
>>> u64 addr, mask;
>>> int ret, idx = 0;
>>> @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
>>> *target,
>>> membuf_store(&to, addr);
>>> membuf_store(&to, mask);
>>> membuf_store(&to, ctrl);
>>> + membuf_zero(&to, sizeof(u32));
>>> idx++;
>>> }
>>> @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
>>> *target,
>>> int ret, idx = 0, offset, limit;
>>> unsigned int note_type = regset->core_note_type;
>>> - /* Resource info */
>>> + /* Resource info and pad */
>>> offset = offsetof(struct user_watch_state, dbg_regs);
>>> user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
>>> offset);
>>> @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
>>> *target,
>>> if (ret)
>>> return ret;
>>> offset += PTRACE_HBP_CTRL_SZ;
>>> +
>>> + user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
>>> + offset, offset +
>>> PTRACE_HBP_PAD_SZ);
>>> + offset += PTRACE_HBP_PAD_SZ;
>>> idx++;
>>> }
>>
>
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
Hi, Xuerui and Ruoyao
On 2023/4/20 上午1:24, WANG Xuerui wrote:
> On 4/19/23 19:00, WANG Xuerui wrote:
>> On 2023/4/19 18:42, Xi Ruoyao wrote:
>>> On Tue, 2023-04-18 at 17:13 +0800, Qing Zhang wrote:
>>>> This is done in order to easily calculate the number of breakpoints
>>>> in hw_break_get.
>>>>
>>>> Signed-off-by: Qing Zhang <[email protected]>
>>>> ---
>>>> arch/loongarch/include/uapi/asm/ptrace.h | 3 ++-
>>>> arch/loongarch/kernel/ptrace.c | 13 +++++++++----
>>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/uapi/asm/ptrace.h
>>>> b/arch/loongarch/include/uapi/asm/ptrace.h
>>>> index 2282ae1fd3b6..06e3be52cb04 100644
>>>> --- a/arch/loongarch/include/uapi/asm/ptrace.h
>>>> +++ b/arch/loongarch/include/uapi/asm/ptrace.h
>>>> @@ -57,11 +57,12 @@ struct user_lasx_state {
>>
>> Drive-by comment to the patch author: there is no "user_lasx_state"
>> yet. Please always state your base commit if not obvious, or you
>> should start from some well-known upstream HEAD (e.g. Linus' rc tags,
>> loongarch-fixes, or loongarch-next).
>>
>>>> };
>>>> struct user_watch_state {
>>>> - uint16_t dbg_info;
>>>> + uint64_t dbg_info;
>>>
>>> Ouch. This is a breaking change when we consider user code like
>>> `printf(PRIu16 "\n", ptr->dbg_info);`. Is it really necessary?
>>
>> Ah right. This is UAPI so without *very* concrete and convicing reason
>> why the change is not going to impact any potential users, it's gonna
>> be a presumed NAK. In other words you must demonstrate (1) why it's
>> absolutely necessary to make the change and (2) that it's impossible
>> to impact anyone, before any such changes can even be considered.
> Please ignore all of this. The memory layout is actually the same after
> the change due to the padding, I was somehow thinking in big-endian a
> few hours ago. (The commit message didn't help either, I think both
> Ruoyao and me got into the habitual thinking that changes like this are
> most likely just churn without real benefits, after *not* seeing the
> rationale in the commit message which was kinda expected.)
>>
This patch does not change the size of the structure. The structure
itself is implicitly aligned. We changed it to explicit alignment for
the convenience of hw_break_get/set (using membuf.left) to calculate the
offset and prevent breaks. Count overflow.
With pad explicit alignment, after membuf_write(&to, &info,
sizeof(info)); to.left=200-8 bytes,
Thus,
membuf_store(&to, addr);
membuf_store(&to, mask);
membuf_store(&to, ctrl);
membuf_zero(&to, sizeof(u32));
After that, to.left is decremented by 24 bytes each time,
so the number of breakpoints will not overflow.
The user support code has not been submitted to the upstream, so
the current uapi change has no effect.
Thanks,
-Qing
>>>
>>>> struct {
>>>> uint64_t addr;
>>>> uint64_t mask;
>>>> uint32_t ctrl;
>>>> + uint32_t pad;
>>>> } dbg_regs[8];
>>>> };
>>>> diff --git a/arch/loongarch/kernel/ptrace.c
>>>> b/arch/loongarch/kernel/ptrace.c
>>>> index 0c7c41e41cad..9c3bc1bbf2ff 100644
>>>> --- a/arch/loongarch/kernel/ptrace.c
>>>> +++ b/arch/loongarch/kernel/ptrace.c
>>>> @@ -475,10 +475,10 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned
>>>> int note_type,
>>>> return 0;
>>>> }
>>>> -static int ptrace_hbp_get_resource_info(unsigned int note_type, u16
>>>> *info)
>>>> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u64
>>>> *info)
>>>> {
>>>> u8 num;
>>>> - u16 reg = 0;
>>>> + u64 reg = 0;
>>>> switch (note_type) {
>>>> case NT_LOONGARCH_HW_BREAK:
>>>> @@ -616,7 +616,7 @@ static int hw_break_get(struct task_struct
>>>> *target,
>>>> const struct user_regset *regset,
>>>> struct membuf to)
>>>> {
>>>> - u16 info;
>>>> + u64 info;
>>>> u32 ctrl;
>>>> u64 addr, mask;
>>>> int ret, idx = 0;
>>>> @@ -646,6 +646,7 @@ static int hw_break_get(struct task_struct
>>>> *target,
>>>> membuf_store(&to, addr);
>>>> membuf_store(&to, mask);
>>>> membuf_store(&to, ctrl);
>>>> + membuf_zero(&to, sizeof(u32));
>>>> idx++;
>>>> }
>>>> @@ -662,7 +663,7 @@ static int hw_break_set(struct task_struct
>>>> *target,
>>>> int ret, idx = 0, offset, limit;
>>>> unsigned int note_type = regset->core_note_type;
>>>> - /* Resource info */
>>>> + /* Resource info and pad */
>>>> offset = offsetof(struct user_watch_state, dbg_regs);
>>>> user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0,
>>>> offset);
>>>> @@ -704,6 +705,10 @@ static int hw_break_set(struct task_struct
>>>> *target,
>>>> if (ret)
>>>> return ret;
>>>> offset += PTRACE_HBP_CTRL_SZ;
>>>> +
>>>> + user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
>>>> + offset, offset +
>>>> PTRACE_HBP_PAD_SZ);
>>>> + offset += PTRACE_HBP_PAD_SZ;
>>>> idx++;
>>>> }
>>>
>>
On Thu, 2023-04-20 at 10:14 +0800, Qing Zhang wrote:
> > > Ah right. This is UAPI so without *very* concrete and convicing reason
> > > why the change is not going to impact any potential users, it's gonna
> > > be a presumed NAK. In other words you must demonstrate (1) why it's
> > > absolutely necessary to make the change and (2) that it's impossible
> > > to impact anyone, before any such changes can even be considered.
> > Please ignore all of this. The memory layout is actually the same after
> > the change due to the padding, I was somehow thinking in big-endian a
> > few hours ago.
No. The problem is not related to big endian or little endian.
Changing the type of this field *can* turn valid user code into
undefined behavior. `printf(PRIu16 "\n", ptr->dbg_info);` is an
undefined behavior if ptr->dbg_info is a int16_t, because the standard
says so, not because the machine may be big endian.
It is a rare case where the ABI is backward-compatible but the API is
incompatible.
Why not just insert "int16_t pad1[3];" after dbg_info?
> > (The commit message didn't help either, I think both
> > Ruoyao and me got into the habitual thinking that changes like this are
> > most likely just churn without real benefits, after *not* seeing the
> > rationale in the commit message which was kinda expected.)
> > >
>
> This patch does not change the size of the structure. The structure
> itself is implicitly aligned. We changed it to explicit alignment for
> the convenience of hw_break_get/set (using membuf.left) to calculate the
> offset and prevent breaks. Count overflow.
>
> With pad explicit alignment, after membuf_write(&to, &info,
> sizeof(info)); to.left=200-8 bytes,
> Thus,
> membuf_store(&to, addr);
> membuf_store(&to, mask);
> membuf_store(&to, ctrl);
> membuf_zero(&to, sizeof(u32));
> After that, to.left is decremented by 24 bytes each time,
> so the number of breakpoints will not overflow.
>
> The user support code has not been submitted to the upstream, so
> the current uapi change has no effect.
The problem is once we put a header into the UAPI directory and make a
Linux kernel release, people may start to use it (maybe in a way we
don't expected).
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Thu, 2023-04-20 at 12:24 +0800, Xi Ruoyao wrote:
> undefined behavior. `printf(PRIu16 "\n", ptr->dbg_info);` is an
> undefined behavior if ptr->dbg_info is a int16_t, because the standard
^^^^^^^ uint64_t
:(
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University