Re: [PATCH] linux-user: Fix msqid_ds struct wrt 32-bit big endian architectures

(Adding Geert as kernel m68k maintainer)

On 8/1/26 22:58, Helge Deller wrote:
> On 8/1/26 14:11, Peter Maydell wrote:
>> On Fri, 31 Jul 2026 at 20:36, Helge Deller <[email protected]> wrote:
>>>
>>> Hello Peter,
>>>
>>> On 7/31/26 20:05, Peter Maydell wrote:
>>>> On Tue, 28 Jul 2026 at 20:32, Helge Deller <[email protected]> wrote:
>>>>>
>>>>> From: Helge Deller <[email protected]>
>>>>>
>>>>> Make sure that the time entries (msg_stime, msg_rtime and msg_ctime)
>>>>> are defined as 64-bit time_t values, since the userspace may access
>>>>> the whole 64-bit value. By this change we fix the word ordering for
>>>>> 32-bit big endian architectures as well.
>>>>>
>>>>> This fixes the msgctl01 LTP testcase on hppa32.
>>>>>
>>>>> Signed-off-by: Helge Deller <[email protected]>
>>>>> ---
>>>>>    linux-user/syscall.c | 30 ++++++++++++------------------
>>>>>    1 file changed, 12 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>>> index 740142825d..c93b770ced 100644
>>>>> --- a/linux-user/syscall.c
>>>>> +++ b/linux-user/syscall.c
>>>>> @@ -4216,21 +4216,15 @@ static inline abi_long do_semtimedop(int semid,
>>>>>    }
>>>>>    #endif
>>>>
>>>> I see this has already gone into git, but some late review
>>>> comments. I suspect this is mostly me being confused rather
>>>> than actual problems.
>>>>
>>>>>
>>>>> +#define target_time64_t         abi_ullong
>>>>> +#define target_swap_time64(x)   tswap64(x)
>>>>> +
>>>>>    struct target_msqid_ds
>>>>
>>>> Is this the kernel's "struct msqid_ds" (which it calls "Obsolete, used
>>>> only for backwards compatibility and libc5 compiles") or its msqid64_ds?
>>>
>>> Yes, it's msqid64_ds.
>>>
>>>> The layout matches msqid64_ds, which makes our struct a bit
>>>> confusingly named.
>>>
>>> True.
>>> In a follow-up patch this better should be renamed to target_msqid64_ds.
>>>
>>>>>    {
>>>>>        struct target_ipc_perm msg_perm;
>>>>> -    abi_ulong msg_stime;
>>>>> -#if TARGET_ABI_BITS == 32
>>>>> -    abi_ulong __unused1;
>>>>> -#endif
>>>>> -    abi_ulong msg_rtime;
>>>>> -#if TARGET_ABI_BITS == 32
>>>>> -    abi_ulong __unused2;
>>>>> -#endif
>>>>> -    abi_ulong msg_ctime;
>>>>> -#if TARGET_ABI_BITS == 32
>>>>> -    abi_ulong __unused3;
>>>>> -#endif
>>>>> +    target_time64_t msg_stime;
>>>>> +    target_time64_t msg_rtime;
>>>>> +    target_time64_t msg_ctime;
>>>>
>>>> Assuming msqid64_ds, the kernel version of this struct has a comment:
>>>>    * 64 bit architectures use a 64-bit long time field here, while
>>>>    * 32 bit architectures have a pair of unsigned long values.
>>>>    * On big-endian systems, the lower half is in the wrong place.
>>>
>>> Those comments are from the generic header:
>>> include/uapi/asm-generic/msgbuf.h
>>> and I think the last sentence about big-endian systems is wrong, as
>>> most platforms provide an own architecture-specific header file, e.g:
>>> arch/xtensa/include/uapi/asm/msgbuf.h
>>> and the big-endian platforms seem to have their high-word first.
>>
>> The comment is correct for any bigendian 32-bit system that
>> uses the kernel's asm-generic structure. Maybe there just aren't
>> any of those that we care about?
> 
> m68k could be a problem.
> The other big-arches seem to be handled correctly by the kernel:
> arch/sparc/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/parisc/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/xtensa/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/x86/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/powerpc/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/mips/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/mips/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> arch/mips/include/uapi/asm/msgbuf.h:struct msqid64_ds {
> 
>>>> That would make tswap64() not the right swap for 32-bit big
>>>> endian guests.
>>>
>>> I did my testing on the 32-bit hppa/parisc platform, and with my patch
>>> the results in qemu were correct and were the same as on physical machines.
>>>
>>> I just tried in a 32-bit powerpc chroot and the msgctl01 now succeeds there as well.
>>> So, I think my patch is generally ok.
>>
>> Yeah, 32-bit powerpc also provides its own structure definition.
>>
>> I think it's worth having a comment about this, at least, because
>> otherwise the obvious assumption is that our struct which has
>> no target-specific ifdefs is supposed to match the kernel's
>> asm-generic struct.
> 
> Yes, I will the testing on m68k, afterwards I can come up with a proposal.
> Maybe it makes sense to modify the kernel comment too....


The msgctl01 testcase from LTP gives this preprocessed code on m68k:
...
# 27 "/usr/include/m68k-linux-gnu/bits/types/struct_msqid_ds.h" 3 4
struct msqid_ds
{

# 1 "/usr/include/m68k-linux-gnu/bits/types/struct_msqid64_ds_helper.h" 1 3 4
# 20 "/usr/include/m68k-linux-gnu/bits/types/struct_msqid64_ds_helper.h" 3 4
   struct ipc_perm msg_perm;
   __time64_t msg_stime;
   __time64_t msg_rtime;
   __time64_t msg_ctime;
   __syscall_ulong_t __msg_cbytes;
   msgqnum_t msg_qnum;
   msglen_t msg_qbytes;
   __pid_t msg_lspid;
   __pid_t msg_lrpid;
   unsigned long int __glibc_reserved4;
   unsigned long int __glibc_reserved5;
# 31 "/usr/include/m68k-linux-gnu/bits/types/struct_msqid_ds.h" 2 3 4
# 53 "/usr/include/m68k-linux-gnu/bits/types/struct_msqid_ds.h" 3 4
};

but the testcase fails with:
msgctl01.c:53: TFAIL: msg_ctime = 6a6e65d900000000, expected 6a6e65d9

So, for m68k either the glibc or the kernel is wrong.
At least they don't agree with each other.

I think it would be beneficial, if the m68k kernel would follow m68k's
glibc implementation of msqid64_ds.

Helge