aarch64: Fix ipc_perm definition for ILP32

  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.
  

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
>>
>