diff mbox series

[1/2] LoongArch: Add pad structure members for explicit alignment

Message ID 20230418091348.9239-1-zhangqing@loongson.cn
State New
Headers
Series [1/2] LoongArch: Add pad structure members for explicit alignment |

Commit Message

Qing Zhang April 18, 2023, 9:13 a.m. UTC 
  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(-)

Comments

kernel test robot April 18, 2023, 5:28 p.m. UTC | #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/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
Xi Ruoyao April 19, 2023, 10:42 a.m. UTC | #2 
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++;
>         }
>
WANG Xuerui April 19, 2023, 11 a.m. UTC | #3 
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 Xuerui April 19, 2023, 5:24 p.m. UTC | #4 
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++;
>>>          }
>>
>
Qing Zhang April 20, 2023, 2:14 a.m. UTC | #5 
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++;
>>>>          }
>>>
>>
Xi Ruoyao April 20, 2023, 4:24 a.m. UTC | #6 
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 April 20, 2023, 4:28 a.m. UTC | #7 
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

:(
diff mbox series

Patch

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++;
 	}