linux-kernel - Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit alignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <48bc7236-638d-9086-daaf-62eacea80dd2@xen0n.name>
Date:   Thu, 20 Apr 2023 01:24:24 +0800
From:   WANG Xuerui <kernel@...0n.name>
To:     Xi Ruoyao <xry111@...111.site>, Qing Zhang <zhangqing@...ngson.cn>,
        Huacai Chen <chenhuacai@...nel.org>
Cc:     Jiaxun Yang <jiaxun.yang@...goat.com>, loongarch@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] LoongArch: Add pad structure members for explicit
 alignment

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@...ngson.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/