* [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment @ 2023-04-18 9:13 Qing Zhang 2023-04-18 17:28 ` kernel test robot 2023-04-19 10:42 ` Xi Ruoyao 0 siblings, 2 replies; 8+ messages in thread From: Qing Zhang @ 2023-04-18 9:13 UTC (permalink / raw) To: Huacai Chen, WANG Xuerui; +Cc: Jiaxun Yang, loongarch, linux-kernel This is done in order to easily calculate the number of breakpoints in hw_break_get. Signed-off-by: Qing Zhang <zhangqing@loongson.cn> --- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-18 9:13 [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment Qing Zhang @ 2023-04-18 17:28 ` kernel test robot 2023-04-19 10:42 ` Xi Ruoyao 1 sibling, 0 replies; 8+ messages in thread From: kernel test robot @ 2023-04-18 17:28 UTC (permalink / raw) To: Qing Zhang, Huacai Chen, WANG Xuerui Cc: oe-kbuild-all, Jiaxun Yang, loongarch, linux-kernel 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/202304190108.aA4uyDQZ-lkp@intel.com/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 <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202304190108.aA4uyDQZ-lkp@intel.com/ 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-18 9:13 [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment Qing Zhang 2023-04-18 17:28 ` kernel test robot @ 2023-04-19 10:42 ` Xi Ruoyao 2023-04-19 11:00 ` WANG Xuerui 1 sibling, 1 reply; 8+ messages in thread From: Xi Ruoyao @ 2023-04-19 10:42 UTC (permalink / raw) To: Qing Zhang, Huacai Chen, WANG Xuerui; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <zhangqing@loongson.cn> > --- > 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 <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-19 10:42 ` Xi Ruoyao @ 2023-04-19 11:00 ` WANG Xuerui 2023-04-19 17:24 ` WANG Xuerui 0 siblings, 1 reply; 8+ messages in thread From: WANG Xuerui @ 2023-04-19 11:00 UTC (permalink / raw) To: Xi Ruoyao, Qing Zhang, Huacai Chen; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <zhangqing@loongson.cn> >> --- >> 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-19 11:00 ` WANG Xuerui @ 2023-04-19 17:24 ` WANG Xuerui 2023-04-20 2:14 ` Qing Zhang 0 siblings, 1 reply; 8+ messages in thread From: WANG Xuerui @ 2023-04-19 17:24 UTC (permalink / raw) To: Xi Ruoyao, Qing Zhang, Huacai Chen; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <zhangqing@loongson.cn> >>> --- >>> 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-19 17:24 ` WANG Xuerui @ 2023-04-20 2:14 ` Qing Zhang 2023-04-20 4:24 ` Xi Ruoyao 0 siblings, 1 reply; 8+ messages in thread From: Qing Zhang @ 2023-04-20 2:14 UTC (permalink / raw) To: WANG Xuerui, Xi Ruoyao, Huacai Chen; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <zhangqing@loongson.cn> >>>> --- >>>> 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++; >>>> } >>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-20 2:14 ` Qing Zhang @ 2023-04-20 4:24 ` Xi Ruoyao 2023-04-20 4:28 ` Xi Ruoyao 0 siblings, 1 reply; 8+ messages in thread From: Xi Ruoyao @ 2023-04-20 4:24 UTC (permalink / raw) To: Qing Zhang, WANG Xuerui, Huacai Chen; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment 2023-04-20 4:24 ` Xi Ruoyao @ 2023-04-20 4:28 ` Xi Ruoyao 0 siblings, 0 replies; 8+ messages in thread From: Xi Ruoyao @ 2023-04-20 4:28 UTC (permalink / raw) To: Qing Zhang, WANG Xuerui, Huacai Chen; +Cc: Jiaxun Yang, loongarch, linux-kernel 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 <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-20 4:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-18 9:13 [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment Qing Zhang 2023-04-18 17:28 ` kernel test robot 2023-04-19 10:42 ` Xi Ruoyao 2023-04-19 11:00 ` WANG Xuerui 2023-04-19 17:24 ` WANG Xuerui 2023-04-20 2:14 ` Qing Zhang 2023-04-20 4:24 ` Xi Ruoyao 2023-04-20 4:28 ` Xi Ruoyao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®