[PATCH v5 3/3] sysv: linux: Pass 64-bit version of semctl syscall
Alistair Francis
alistair23@gmail.com
Tue Apr 28 21:59:51 GMT 2020
On Wed, Apr 22, 2020 at 2:26 PM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 21/04/2020 17:52, Stepan Golosunov wrote:
> > 01.04.2020 в 09:53:08 -0700 Alistair Francis написал:
> >> The semctl_syscall() function passes a union semun to the kernel. The
> >> union includes struct semid_ds as a member. On 32-bit architectures the
> >> Linux kernel provides a *_high version of the 32-bit sem_otime and
> >> sem_ctime values. These can be combined to get a 64-bit version of the
> >> time.
> >>
> >> This patch adjusts the struct semid_ds to support the *_high versions
> >> of sem_otime and sem_ctime. For 32-bit systems with a 64-bit time_t
> >> this can be used to get a 64-bit time from the two 32-bit values.
> >>
> >> Due to allignment differences between 64-bit and 32-bit variables we
> >> also need to set nsems to ensure it's correct.
> >> ---
> >> .../unix/sysv/linux/hppa/struct__semid_ds32.h | 32 +++++++++++++++++++
> >> .../unix/sysv/linux/mips/struct__semid_ds32.h | 30 +++++++++++++++++
> >> .../sysv/linux/powerpc/struct__semid_ds32.h | 32 +++++++++++++++++++
> >> sysdeps/unix/sysv/linux/semctl.c | 30 ++++++++++++++---
> >> .../sysv/linux/sparc/struct__semid_ds32.h | 32 +++++++++++++++++++
> >> sysdeps/unix/sysv/linux/struct__semid_ds32.h | 32 +++++++++++++++++++
> >> .../unix/sysv/linux/x86/struct__semid_ds32.h | 32 +++++++++++++++++++
> >> 7 files changed, 216 insertions(+), 4 deletions(-)
> >> create mode 100644 sysdeps/unix/sysv/linux/hppa/struct__semid_ds32.h
> >> create mode 100644 sysdeps/unix/sysv/linux/mips/struct__semid_ds32.h
> >> create mode 100644 sysdeps/unix/sysv/linux/powerpc/struct__semid_ds32.h
> >> create mode 100644 sysdeps/unix/sysv/linux/sparc/struct__semid_ds32.h
> >> create mode 100644 sysdeps/unix/sysv/linux/struct__semid_ds32.h
> >> create mode 100644 sysdeps/unix/sysv/linux/x86/struct__semid_ds32.h
> >
> >> diff --git a/sysdeps/unix/sysv/linux/semctl.c b/sysdeps/unix/sysv/linux/semctl.c
> >> index 30571af49f..b0e4d828ed 100644
> >> --- a/sysdeps/unix/sysv/linux/semctl.c
> >> +++ b/sysdeps/unix/sysv/linux/semctl.c
> >> @@ -22,6 +22,7 @@
> >> #include <sysdep.h>
> >> #include <shlib-compat.h>
> >> #include <errno.h>
> >> +#include <struct__semid_ds32.h>
> >> #include <linux/posix_types.h> /* For __kernel_mode_t. */
> >>
> >> /* Define a `union semun' suitable for Linux here. */
> >> @@ -29,6 +30,9 @@ union semun
> >> {
> >> int val; /* value for SETVAL */
> >> struct semid_ds *buf; /* buffer for IPC_STAT & IPC_SET */
> >> +#if __WORDSIZE == 32
> >> + struct __semid_ds32 *buf32; /* 32-bit buffer for IPC_STAT */
> >> +#endif
> >
> > This won't compile on x32.
>
> In fact it will, since the incomplete type won't be actually used on x32
> and c99 struct initialization does not catch this (at least this patch
> set does build with gcc 9.3 for x32). But I agree that it not correct
> to actually define it for x32.
>
> >
> >> unsigned short int *array; /* array for GETALL & SETALL */
> >> struct seminfo *__buf; /* buffer for IPC_INFO */
> >> };
> >> @@ -44,13 +48,31 @@ union semun
> >> static int
> >> semctl_syscall (int semid, int semnum, int cmd, union semun arg)
> >> {
> >> + int ret;
> >> #ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS
> >> - return INLINE_SYSCALL_CALL (semctl, semid, semnum, cmd | __IPC_64,
> >> - arg.array);
> >> + ret = INLINE_SYSCALL_CALL (semctl, semid, semnum, cmd | __IPC_64,
> >> + arg.array);
> >> #else
> >> - return INLINE_SYSCALL_CALL (ipc, IPCOP_semctl, semid, semnum, cmd | __IPC_64,
> >> - SEMCTL_ARG_ADDRESS (arg));
> >> + ret = INLINE_SYSCALL_CALL (ipc, IPCOP_semctl, semid, semnum, cmd | __IPC_64,
> >> + SEMCTL_ARG_ADDRESS (arg));
> >> #endif
> >> +
> >> +#if (__WORDSIZE == 32 && __TIMESIZE == 64 \
> >> + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> >> + if (ret == 0 && (cmd == IPC_STAT || cmd == SEM_STAT || cmd == SEM_STAT_ANY))
> >> + {
> >> + struct semid_ds temp_ds = (struct semid_ds) {
> >> + .sem_perm = arg.buf32->sem_perm,
> >> + .sem_nsems = arg.buf32->sem_nsems,
> >> + .sem_otime = (arg.buf32->sem_otime
> >> + | ((time_t) arg.buf32->sem_otime_high << 32)),
> >> + .sem_ctime = (arg.buf32->sem_ctime
> >> + | ((time_t) arg.buf32->sem_ctime_high << 32)),
> >> + };
> >> + *arg.buf = temp_ds;
> >> + }
> >> +#endif
> >> + return ret;
> >> }
> >
> > You do not need 2 temporary variables (temp_ds and an anonymous one).
> >
> > ….buf = (struct semid_ds) { … };
> > or
> > struct semid_ds temp_ds = { … };
> >
> >
> > And while I am not too familiar with aliasing rules this still seems
> > to violate them (accessing the same memory via 2 pointers of
> > incompatible types). Making this an undefined behavior.
>
> It uses a temporary buffer exactly to avoid it (similar to mips64
> getdents64).
>
> However, the way previous the patch redefined semid_ds for
> __TIMESIZE == 64 does not take in consideration the 64-bit __time_t
> alignment (it might be case where sem_perm force padding on next
> element). It makes the glibc exported semid_ds incompatible with
> kernel one, which is not an issues but prevent glibc to pass it
> directly to kernel in the syscall.
>
> And there is another issue where prior v4.14 (commit
> 4693916846269d633a3664586650dbfac2c5562f) Linux left the reserved
> space (used later to the high bits of 64-bit times) untouched rather
> than zeroed. This means that the caller must zero the kernel passed
> buffer.
>
> So both issues prevents to use the user provided buffer directly in
> the syscall. So what I think it would more straightforward to use a
> temporary to issue the syscall.
>
> Something like:
>
> #define IPC_TIME64 \
> (__WORDSIZE == 32 && __TIMESIZE == 64 \
> && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
>
> static int
> semctl_syscall (int semid, int semnum, int cmd, union semun arg)
> {
> int ret;
> #if IPC_TIME64
> struct __semid_ds32 tmp, *orig;
> if (cmd == IPC_SET)
> {
> tmp = (struct __semid_ds32) {
> .sem_perm = arg.buf->sem_perm,
> .sem_otime = arg.buf->sem_otime,
> .sem_ctime = arg.buf->sem_ctime,
> .sem_nsems = arg.buf->sem_nsems,
> };
> orig = arg.buf;
> arg.buf = &tmp;
> }
> #endif
>
> #ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS
> ret = INLINE_SYSCALL_CALL (semctl, semid, semnum, cmd | __IPC_64,
> arg.array);
> #else
> ret = INLINE_SYSCALL_CALL (ipc, IPCOP_semctl, semid, semnum, cmd | __IPC_64,
> SEMCTL_ARG_ADDRESS (arg));
> #endif
>
> #if IPC_TIME64
> if (r >= 0
> && (cmd == IPC_STAT || cmd == SEM_STAT || cmd == SEM_STAT_ANY))
> {
> arg.buf = orig;
> arg.buf->sem_perm = tmp.sem_perm;
> arg.buf->sem_otime = tmp.sem_otime
> | ((time_t) tmp.sem_otime_high << 32);
> arg.buf->sem_ctime = tmp.sem_ctime
> | ((time_t) tmp.sem_ctime_high << 32);
> arg.buf->sem_nsems = tmp.sem_nsems;
> }
> #endif
>
> (the first part where the user provided buffer might require some adjustments
> once __TIMESIZE == 64 is being exported in public ABI).
With a few changes I used what you posted above. Sending a new version now.
Thanks for the help.
Alistair
>
> >
> > What I was suggesting is something like this:
> >
> > union semun
> > {
> > int val;
> > union { struct semid_ds s, struct __semid_ds32 s32 } *buf;
> > unsigned short int *array;
> > struct seminfo *__buf;
> > };
> >
> > Or even with an anonymous union member:
> >
> > union __semid_ds {
> > struct semid_ds;
> > #if …
> > struct __semid_ds32 buf32;
> > #endif
> > }
> > union semun
> > {
> > int val;
> > union __semid_ds *buf;
> > unsigned short int *array;
> > struct seminfo *__buf;
> > };
> >
> > This should allow to access both structures simultaneously without
> > local violations of aliasing rules. (I think that aliasing violations
> > between different compilation units are normal in glibc. While inside
> > a single file they can lead to miscompilation.)
> >
> >
> >> diff --git a/sysdeps/unix/sysv/linux/x86/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/x86/struct__semid_ds32.h
> >> new file mode 100644
> >> index 0000000000..4e4ab26661
> >> --- /dev/null
> >> +++ b/sysdeps/unix/sysv/linux/x86/struct__semid_ds32.h
> >> @@ -0,0 +1,32 @@
> >> +/* Sparc implementation of the semaphore struct __semid_ds32
> >> + Copyright (C) 1995-2020 Free Software Foundation, Inc.
> >> + This file is part of the GNU C Library.
> >> +
> >> + The GNU C Library is free software; you can redistribute it and/or
> >> + modify it under the terms of the GNU Lesser General Public
> >> + License as published by the Free Software Foundation; either
> >> + version 2.1 of the License, or (at your option) any later version.
> >> +
> >> + The GNU C Library is distributed in the hope that it will be useful,
> >> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + Lesser General Public License for more details.
> >> +
> >> + You should have received a copy of the GNU Lesser General Public
> >> + License along with the GNU C Library; if not, see
> >> + <https://www.gnu.org/licenses/>. */
> >> +
> >> +#ifdef __i386__
> >> +/* This is the "new" y2038 types defined after the 5.1 kernel. It allows
> >> + * the kernel to use {o,c}time{_high} values to support a 64-bit time_t. */
> >> +struct __semid_ds32 {
> >> + struct ipc_perm sem_perm; /* operation permission struct */
> >> + __syscall_ulong_t sem_otime; /* last semop() time */
> >> + __syscall_ulong_t sem_otime_high; /* last semop() time high */
> >> + __syscall_ulong_t sem_ctime; /* last time changed by semctl() */
> >> + __syscall_ulong_t sem_ctime_high; /* last time changed by semctl() high */
> >> + __syscall_ulong_t sem_nsems; /* number of semaphores in set */
> >> + __syscall_ulong_t __glibc_reserved3;
> >> + __syscall_ulong_t __glibc_reserved4;
> >> +};
> >> +#endif
More information about the Libc-alpha
mailing list