This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] aarch64: Fix ipc_perm definition for ILP32

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://
>>>>> 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:
>>> (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
> 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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]