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

[Rich Felker <dalias at libc dot org>]

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.

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