Message ID | 1503423981.28672.31.camel@cavium.com |
---|---|
State | New |
Headers | show |
Hi Steve, On Tue, Aug 22, 2017 at 10:46:21AM -0700, Steve Ellcey wrote: > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > This change is only needed for ILP32 so it doesn't need to go in until we add > that ABI but I am sending out for review and comments. > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > Steve Ellcey <sellcey@cavium.com> > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > Ifdef and pad the mode field for ILP32. > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > x/aarch64/bits/ipc.h > index cd1f06e..cd05b74 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > @@ -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 This is my typo. It should be #ifdef __LP64__ > unsigned int mode; /* Read/write permission. */ > +#else > + unsigned short int mode; /* Read/write permission. */ > + unsigned short int __pad0; > +#endif > unsigned short int __seq; /* Sequence number. */ > unsigned short int __pad1; > __syscall_ulong_t __glibc_reserved1;
On 22/08/17 18:46, Steve Ellcey wrote: > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > This change is only needed for ILP32 so it doesn't need to go in until we add > that ABI but I am sending out for review and comments. > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > Steve Ellcey <sellcey@cavium.com> > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > Ifdef and pad the mode field for ILP32. > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > x/aarch64/bits/ipc.h > index cd1f06e..cd05b74 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > @@ -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.
On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: > On 22/08/17 18:46, Steve Ellcey wrote: > > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > > This change is only needed for ILP32 so it doesn't need to go in until we add > > that ABI but I am sending out for review and comments. > > > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > > Steve Ellcey <sellcey@cavium.com> > > > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > > Ifdef and pad the mode field for ILP32. > > > > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > > x/aarch64/bits/ipc.h > > index cd1f06e..cd05b74 100644 > > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > > @@ -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; 254 unsigned short seq; 255 unsigned short __pad2; 256 compat_ulong_t unused1; 257 compat_ulong_t unused2; 258 }; So I would prefer to treat it not as the change of ABI, bit the fix of ABI incompatibility on GLIBC side. Yury
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.
On 23/08/17 11:37, Szabolcs Nagy wrote: > 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. > hm it seems to me that a mode_t change would be very intrusive.. can we keep the ipc_perm mode field unsigned int and do endian fixup/zero pad in the syscall interface?
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. 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
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 >
On 23/08/17 12: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. > ok, it seems it's one of the posix conformance issues that affect other targets too https://sourceware.org/bugzilla/show_bug.cgi?id=18231 so it's ok to have this issue on ilp32 too. > 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 >
On 23/08/2017 09:42, Szabolcs Nagy wrote: > On 23/08/17 12: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. >> > > ok, it seems it's one of the posix conformance issues > that affect other targets too > > https://sourceware.org/bugzilla/show_bug.cgi?id=18231 > > so it's ok to have this issue on ilp32 too. I think it is feasible to fix it on generic ipc.h and let the required architecture to redefine it as required for their own kernel ABIs. > >> 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 >> >
diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu x/aarch64/bits/ipc.h index cd1f06e..cd05b74 100644 --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h @@ -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 unsigned short int __seq; /* Sequence number. */ unsigned short int __pad1; __syscall_ulong_t __glibc_reserved1;