Re: [PATCH v2 4/4] sysvipc: Set ipc_perm mode as mode_t (BZ#18231)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 27/08/2019 11:30, Rich Felker wrote:
> On Tue, Jul 30, 2019 at 09:30:30AM -0300, Adhemerval Zanella wrote:
>> Changes from previous version:
>>
>>   - Add m68k ABIs.
>>
>>   - XFAIL ipc_perm mode on conform/data/sys/ipc.h-data just for Hurd.
>>
>> --
>>
>> This patch sets the mode field in ipc_perm as mode_t for all architectures,
>> as POSIX specification [1].  The changes required are as follow:
>>
>>   1. It moves the ipc_perm definition out of ipc.h to its own header
>>      ipc_perm.h.  It also allows consolidate the IPC_* definition on
>>      only one header.
>>
>>   2. The generic implementation follow the kernel ipc64_perm size so the
>>      syscall can be made directly without temporary buffer copy.  However,
>>      since glibc defines the MODE field as mode_t, it omits the __PAD1 field
>>      (since glibc does not export mode_t as 16-bit for any architecture).
>>
>>      It is a two-fold improvement:
>>
>>      2.1. New implementation which follow Linux UAPI will not need to
>> 	  provide an arch-specific ipc-perm.h header neither wrongly
>>           use the wrong 16-bit definition from previous default ipc.h
>> 	  (as csky did).
>>
>>      2.1. It allows consolidate ipc_perm definition for architectures that
>>           already provide mode_t as 32-bit.
>>
>>   3. All kernel ABIs for the supported architectures already provides the
>>      expected padding for mode type extension to 32-bit.  However, some
>>      architectures the padding has the wrong placement, so it requires
>>      the ipc control routines (msgctl, semctl, and shmctl) to adjust the
>>      mode field accordingly.  Currently they are armeb, microblaze, m68k,
>>      s390, and sheb.
>>
>>      A new assume is added, __ASSUME_SYSVIPC_SUPPORT_MODE32, which the
>>      required architecture undefine.
>>
>>   4. For the architecture that undefined __ASSUME_SYSVIPC_SUPPORT_MODE32,
>>      it also requires compat symbols that do not adjust the mode field.
> 
> Overall this looks good to me. I find the macro name
> __ASSUME_SYSVIPC_SUPPORT_MODE32 confusing though; my understanding of
> "assume" in glibc conventions is that it's just a hint that fallbacks
> aren't needed, not a hard change in behavior. Here, you've used
> __ASSUME_SYSVIPC_SUPPORT_MODE32 to mean
> !__ARCH_HAS_REVERSED_HALFWORD_IPC_PERM_MODE. I think it would be a lot
> more readable to have the archs that need handling for broken mode
> field define a macro indicating that the syscall interface has it
> backwards, rather than the default assumption being that it's
> backwards.
> 
> For comparison, musl has a macro named SYSCALL_IPC_BROKEN_MODE for
> the affected archs.

We also use __ASSUME macros to define arch-specific kernel ABI for
some syscalls (fadvise or clone for instance).  I don't have a strong
opinion here and I see that by not checking the required macro enable
compat mode is somewhat confusing.

I don't think __ARCH_HAS_REVERSED_HALFWORD_IPC_PERM_MODE really
express the issue (also for other sysvipc idiosyncrasies glibc uses
SYSVIPC on its name).  So I think the macro __ARCH_SYSVIPC_BROKEN_MODE_T
is a better name. 

I will updating the patch to take in consideration newer symbols since
2.31 release and I will send an updated version.

> 
> In any case, as noted on bz #18231, I'd really like to see this
> applied so that the same bug doesn't keep getting copied to new archs.
> 
> Rich
> 

Thanks for checking on it.