This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] aarch64: Fix ipc_perm definition for ILP32
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Wed, 23 Aug 2017 09:34:33 -0300
- Subject: Re: [PATCH] aarch64: Fix ipc_perm definition for ILP32
- Authentication-results: sourceware.org; auth=none
- References: <1503423981.28672.31.camel@cavium.com> <599D50F6.6060009@arm.com> <20170823101023.uvc2ehejstemzxyz@yury-thinkpad> <599D5AEC.7090402@arm.com> <20170823113118.oku24molb6qs5d2q@yury-thinkpad>
On 23/08/2017 08:31, Yury Norov wrote:
> On Wed, Aug 23, 2017 at 11:37:32AM +0100, Szabolcs Nagy wrote:
>> On 23/08/17 11:10, Yury Norov wrote:
>>> On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote:
>>>> On 22/08/17 18:46, Steve Ellcey wrote:
>>>>> @@ -46,7 +46,12 @@ struct ipc_perm
>>>>> __gid_t gid; /* Owner's group ID. */
>>>>> __uid_t cuid; /* Creator's user ID. */
>>>>> __gid_t cgid; /* Creator's group ID. */
>>>>> +#ifdef __LP64
>>>>> unsigned int mode; /* Read/write permission. */
>>>>> +#else
>>>>> + unsigned short int mode; /* Read/write permission. */
>>>>> + unsigned short int __pad0;
>>>>> +#endif
>>>>
>>>> when did this happen?
>>>>
>>>> as far as i can tell staging/ilp32-4.12 branch in
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
>>>> still has unsigned int __kernel_mode_t in uapi, so this is
>>>> an abi change compared to that branch.
>>>>
>>>> i guess it's for 32bit compat, but i'd like to see the
>>>> kernel patch for this.
>>>
>>> It is there from the very beginning of ILP32 patches. The patch changes
>>> the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct
>>> with and without ilp32 patches and this fix.
>>>
>>> To reproduce:
>>> struct semid_ds seminfo;
>>> static char name[] = "Hello";
>>>
>>> key_t key = ftok (name, 'G');
>>> semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644);
>>> semctl (semid, 0, IPC_STAT, &seminfo);
>>> printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage
>>>
>>> On kernel side no fixes needed because arm64/ilp32 ipc requests are
>>> wired to compat layer, and the layout for structure is like this:
>>> arch/arm64/include/asm/compat.h:
>>> 246 struct compat_ipc64_perm {
>>> 247 compat_key_t key;
>>> 248 __compat_uid32_t uid;
>>> 249 __compat_gid32_t gid;
>>> 250 __compat_uid32_t cuid;
>>> 251 __compat_gid32_t cgid;
>>> 252 unsigned short mode;
>>> 253 unsigned short __pad1;
>>
>> we have to decide if mode_t is unsigned int or short on ilp32,
>> changing just the ipc_perm struct in libc is nonconforming:
>> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html
>>
>> (there are some existing conformance issues like that in
>> glibc/linux but we should try to avoid introducing new ones)
>>
>> i think the ilp32 linux uapi should typedef __kernel_mode_t
>> to unsigned short, but i don't know the effect of that on
>> the kernel, so please discuss this with the kernel folks.
>
> The __mode_t is u32 for all glibc ports including arm64, but the problem is
> that the declaration of struct ipc_perm in sysdeps/unix/sysv/linux/bits/ipc.h
> doesn't use __mode_t type, but unsigned long or short.
>
> On kernel side, __kernel_mode_t is already defined, and for arm64 it
> is u32. And like for glibc, compat layer doesn't use __kernel_mode_t,
> but unsigned short.
>
> (For arm, the __kernel_mode_t is unsigned short, while __MODE_T_TYPE
> on glibc side is still __U32_TYPE. I think this is wrong...)
>
> So for me it looks like both glibc and kernel ignore POSIX standard in
> this case: declare mode not as mode_t type in the ipc_perm structure.
> And this is the root of problem. Fixing it is non-trivial task because
> mode_t is not unsigned short or long in general, and so other structures
> will be affected.
The old SysV kernel ABI interface is messy and kernel does not help us
in this manner. The '__kernel_mode_t' is defined on kernel sources as:
arch/arm/include/uapi/asm/posix_types.h 22 typedef unsigned short __kernel_mode_t;
arch/m68k/include/uapi/asm/posix_types.h 10 typedef unsigned short __kernel_mode_t;
arch/microblaze/include/uapi/asm/posix_types.h 4 typedef unsigned short __kernel_mode_t;
arch/parisc/include/uapi/asm/posix_types.h 11 typedef unsigned short __kernel_mode_t;
arch/s390/include/uapi/asm/posix_types.h 25 typedef unsigned short __kernel_mode_t;
arch/s390/include/uapi/asm/posix_types.h 34 typedef unsigned int __kernel_mode_t;
arch/sh/include/uapi/asm/posix_types_32.h 4 typedef unsigned short __kernel_mode_t;
arch/sh/include/uapi/asm/posix_types_64.h 4 typedef unsigned short __kernel_mode_t;
arch/sparc/include/uapi/asm/posix_types.h 36 typedef unsigned short __kernel_mode_t;
arch/x86/include/uapi/asm/posix_types_32.h 10 typedef unsigned short __kernel_mode_t;
include/uapi/asm-generic/posix_types.h 23 typedef unsigned int __kernel_mode_t;
(I excluded non glibc supported architectures)
Basically old 32-bit ABIs uses unsigned short and newer ABI and 64 bits
uses unsigned int. Also, to avoid adding another entrypoint for these
old interfaces kernel added IPC_64 to indicate support to 32-bit UIDs:
arch/aarch64/bits/ipc.h 14 #define IPC_64 0
arch/generic/bits/ipc.h 13 #define IPC_64 0x100
arch/mips64/bits/ipc.h 14 #define IPC_64 0x100
arch/powerpc/bits/ipc.h 14 #define IPC_64 0x100
arch/powerpc64/bits/ipc.h 14 #define IPC_64 0x100
arch/s390x/bits/ipc.h 14 #define IPC_64 0x100
arch/x32/bits/ipc.h 13 #define IPC_64 0
arch/x86_64/bits/ipc.h 13 #define IPC_64 0
(I excluded non glibc supported architectures)
However IPC_64 only make sense for ABI that used to provide 16 bits uids
and then added supported to 32 bits (that's why x86_64/AARCh64 IPC_64 is set
to 0). This is kinda messy because for newer ports is exactly the contrary
(default to a 0x100).
Anyhow, on current GLIBC we use only 32 bits uids as default (even for
old ABIs):
io/fcntl.h 50 typedef __mode_t mode_t;
io/sys/stat.h 59 typedef __mode_t mode_t;
misc/sys/mman.h 37 typedef __mode_t mode_t;
posix/sys/types.h 70 typedef __mode_t mode_t;
sysvipc/sys/ipc.h 38 typedef __mode_t mode_t;
posix/bits/types.h 138 __STD_TYPE __MODE_T_TYPE __mode_t;
bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE
mach/hurd/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE
linux/alpha/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE
linux/generic/bits/typesizes.h 35 #define __MODE_T_TYPE __U32_TYPE
linux/s390/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE
linux/sparc/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE
linux/x86/bits/typesizes.h 43 #define __MODE_T_TYPE __U32_TYPE
Old uid 16 bits is used solely on compat symbols. And that's why we
explicit define the types to unsigned/short instead of using mode_t.
I think it feasible to change ipc_perm to use 32 mode_t for all
architectures, but it means that we will need to actually make a
struct copy on architectures with 16 bits uids that do not support
IPC_64 (and unfortunately we still have some).
Now for ILP32 I see that it tries to use internal ARM code as much
as possible, so using 32-bits mode_t might require a lot of internal
kernel refactor. Would it be possible at least to have IPC_64 support
then? I presume this would require add support for ARM as well.
>
> For arm64/ilp32, we can continue as-is on glibc side, but it will
> require rework on kernel side. If it was only little-endian code, we
> would simply zero pad1, and it would be enough. But there is big-endian
> support for arm64/ilp32, and therefore we need to introduce wrappers
> to swap half-words.
>
> I can prepare RFC that does it, but I think that the problem is
> generic, and so the generic fix required.
>
> Yury
>