[PATCH v7 2/2] sysv: linux: Pass 64-bit version of semctl syscall
Alistair Francis
alistair23@gmail.com
Sat May 9 18:08:23 GMT 2020
On Thu, May 7, 2020 at 12:44 PM Stepan Golosunov <stepan@golosunov.pp.ru> wrote:
>
> On Tue, May 05, 2020 at 08:47:36AM -0700, Alistair Francis wrote:
> > 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.
> >
> > This change handles issues that would arrise with architectures that
> > might select __TIMESIZE at build time (and result in different size
> > semid_ds structs).
> >
> > As described by Adhemerval:
> > 1. If an ABI issues 'semctl (semid, semnum, IPC_STAT, ...)' with
> > IPC_STAT without _IPC_CMD_TIME_64, the provided semid_ds buffer
> > will be used directly on the syscall. This is the case for
> > 64-bit architectures and 32-bit architecture without
> > __TIMESIZE==32 support (RV32 for instance).
> >
> > 2. If an ABI issues 'semctl (semid, semnum, IPC_STAT, ...)' with
> > IPC_STAT with _IPC_CMD_TIME_64 then __TIMESIZE==64 is implied.
> > A temporary __semid_ds32 will be used to issue the syscall and its
> > contents will be copied to users semid_ds buffer (which is also
> > implied to be the __TIMESIZE==64).
>
> As discussed elsewhere, __TIMESIZE description is incorrect here. It's
> part of the abi and cannot be selected at build time.
Yep, but I'm a little confused what else to use.
We should only be using the *_high variables on 32-bit systems with a
64-bit time_t. So we need someway to check if that is the case.
I could change the macros to only work for _WORDSIZE == 32 and then
have a dynamic check on sizeof(time_t) == 8. Would that work?
>
> (And __IPC_CMD_TIME_64 is not actually required for __TIMESIZE==64
> architectures like rv32. If it is used on them it's just for
> consistency. Otherwise those architectures can be covered by
> ((cmd & __IPC_CMD_TIME64) || (__TIMESIZE == 64))
> conditions inside #if __IPC_TIME64 blocks.
>
> Or __IPC_CMD_TIME_64 could be not used at all. __TIMESIZE==32 support
> can be implemented with the usual thin wrapper around __semctl64.
> I suspect this would be preferable if not too costly.)
This isn't really the same as the usual wrappers though as there is no
semctl64 syscall here.
>
> > ---
> > bits/ipctypes.h | 10 ++
> > sysdeps/mips/bits/ipctypes.h | 10 ++
> > sysdeps/unix/sysv/linux/bits/ipc.h | 3 +-
> > sysdeps/unix/sysv/linux/bits/sem.h | 4 +-
> > sysdeps/unix/sysv/linux/ipc_priv.h | 4 +
> > sysdeps/unix/sysv/linux/semctl.c | 121 ++++++++++++++++----
> > sysdeps/unix/sysv/linux/x86/bits/ipctypes.h | 13 +++
> > 7 files changed, 141 insertions(+), 24 deletions(-)
>
> > diff --git a/sysdeps/unix/sysv/linux/semctl.c b/sysdeps/unix/sysv/linux/semctl.c
> > index 30571af49f..99ad686194 100644
> > --- a/sysdeps/unix/sysv/linux/semctl.c
> > +++ b/sysdeps/unix/sysv/linux/semctl.c
> > @@ -22,12 +22,16 @@
> > #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. */
> > union semun
> > {
> > int val; /* value for SETVAL */
> > +#if __IPC_TIME64
> > + struct __semid_ds32 *buf32;
> > +#endif
> > struct semid_ds *buf; /* buffer for IPC_STAT & IPC_SET */
> > unsigned short int *array; /* array for GETALL & SETALL */
> > struct seminfo *__buf; /* buffer for IPC_INFO */
> > @@ -44,13 +48,54 @@ union semun
> > static int
> > semctl_syscall (int semid, int semnum, int cmd, union semun arg)
> > {
> > + int ret;
> > +#if __IPC_TIME64
> > + /* A temporary buffer is used to avoid both an issue where the export
> > + semid_ds might not follow the kernel's expected layout (due
> > + ? to {o,c}time{_high} alignment in 64-bit time case) and the issue where
> > + ? ?some kernel versions might not clear the high bits when returning
> > + ? ?then {o,c}time{_high} information. */
> > + struct __semid_ds32 tmp;
> > + struct semid_ds *orig;
> > + bool restore = false;
> > + if (cmd == IPC_STAT || cmd == SEM_STAT || cmd == SEM_STAT_ANY)
> > + {
> > + tmp = (struct __semid_ds32) {
> > + .sem_perm = arg.buf->sem_perm,
> > + .sem_otime = arg.buf->sem_otime,
> > + .sem_otime_high = arg.buf->sem_otime >> 32,
> > + .sem_ctime = arg.buf->sem_ctime,
> > + .sem_ctime_high = arg.buf->sem_ctime >> 32,
> > + .sem_nsems = arg.buf->sem_nsems,
> > + };
>
> sem_otime_high and sem_ctime_high need to be 0 for compatibility with
> older kernels. And it seems this struct is write-only in kernel, so
> it does not not matter what's in the other fields.
>
> But it looks like all the changes to semctl_syscall function are junk
> from old patch versions as necessary conversions are added to the
> caller.
Yep, I have remove this.
>
> > + orig = arg.buf;
> > + arg.buf = (struct semid_ds*) &tmp;
> > + restore = true;
> > + }
> > +#endif
> > +
> > #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 __IPC_TIME64
> > + if (ret >= 0 && restore)
> > + {
> > + arg.buf = orig;
> > + arg.buf->sem_perm = tmp.sem_perm;
> > + arg.buf->sem_otime = tmp.sem_otime
> > + | ((__time64_t) tmp.sem_otime_high << 32);
> > + arg.buf->sem_ctime = tmp.sem_ctime
> > + | ((__time64_t) tmp.sem_ctime_high << 32);
> > + arg.buf->sem_nsems = tmp.sem_nsems;
> > + }
> > +#endif
> > +
> > + return ret;
> > }
> >
> > int
> > @@ -64,15 +109,15 @@ __new_semctl (int semid, int semnum, int cmd, ...)
> > union semun arg = { 0 };
> > va_list ap;
> >
> > - /* Get the argument only if required. */
> > - switch (cmd)
> > + /* Get the argument only if required. Ignore the __IPC_CMD_TIME64. */
> > + switch (cmd & ~__IPC_CMD_TIME64)
> > {
> > case SETVAL: /* arg.val */
> > case GETALL: /* arg.array */
> > case SETALL:
> > - case IPC_STAT: /* arg.buf */
> > + case IPC_STAT & ~__IPC_CMD_TIME64: /* arg.buf */
> > case IPC_SET:
> > - case SEM_STAT:
> > + case SEM_STAT & ~__IPC_CMD_TIME64:
> > case IPC_INFO: /* arg.__buf */
> > case SEM_INFO:
> > va_start (ap, cmd);
> > @@ -81,6 +126,20 @@ __new_semctl (int semid, int semnum, int cmd, ...)
> > break;
> > }
> >
> > +#ifdef __IPC_TIME64
>
> s/ifdef/if/ here and later.
Good catch, fixed.
Alistair
>
>
> > + struct __semid_ds32 tmp32;
> > + struct semid_ds *orig64;
> > + bool restore = false;
> > +
> > + if (cmd & __IPC_CMD_TIME64)
> > + {
> > + tmp32 = (struct __semid_ds32) { 0 };
> > + orig64 = (struct semid_ds *) arg.buf;
> > + arg.buf = (struct semid_ds *) &tmp32;
> > + restore = true;
> > + }
> > +#endif
> > +
> > #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
> > struct semid_ds tmpds;
> > if (cmd == IPC_SET)
> > @@ -91,26 +150,46 @@ __new_semctl (int semid, int semnum, int cmd, ...)
> > }
> > #endif
> >
> > - int ret = semctl_syscall (semid, semnum, cmd, arg);
> > + int ret = semctl_syscall (semid, semnum, cmd & ~__IPC_CMD_TIME64, arg);
> > + if (ret < 0)
> > + return ret;
> >
> > - if (ret >= 0)
> > + switch (cmd & ~__IPC_CMD_TIME64)
> > {
> > - switch (cmd)
> > - {
> > - case IPC_STAT:
> > - case SEM_STAT:
> > - case SEM_STAT_ANY:
> > + case IPC_STAT:
> > + case SEM_STAT:
> > + case SEM_STAT_ANY:
> > #ifdef __ASSUME_SYSVIPC_BROKEN_MODE_T
> > - arg.buf->sem_perm.mode >>= 16;
> > +# if __IPC_TIME64
> > + arg.buf32->sem_perm.mode >>= 16;
> > +# else
> > + arg.buf->sem_perm.mode >>= 16;
> > +# endif
> > #else
> > - /* Old Linux kernel versions might not clear the mode padding. */
> > - if (sizeof ((struct semid_ds){0}.sem_perm.mode)
> > - != sizeof (__kernel_mode_t))
> > - arg.buf->sem_perm.mode &= 0xFFFF;
> > + /* Old Linux kernel versions might not clear the mode padding. */
> > + if (sizeof ((struct semid_ds){0}.sem_perm.mode)
> > + != sizeof (__kernel_mode_t))
> > +# if __IPC_TIME64
> > + arg.buf32->sem_perm.mode &= 0xFFFF;
> > +# else
> > + arg.buf->sem_perm.mode &= 0xFFFF;
> > +# endif
> > #endif
> > - }
> > }
> >
> > +#ifdef __IPC_TIME64
> > + if (restore)
> > + {
> > + orig64->sem_perm = tmp32.sem_perm;
> > + orig64->sem_otime = tmp32.sem_otime
> > + | ((__time64_t) tmp32.sem_otime_high << 32);
> > + orig64->sem_ctime = tmp32.sem_ctime
> > + | ((__time64_t) tmp32.sem_ctime_high << 32);
> > + orig64->sem_nsems = tmp32.sem_nsems;
> > + arg.buf = (struct semid_ds *) orig64;
> > + }
> > +#endif
> > +
> > return ret;
> > }
> > versioned_symbol (libc, __new_semctl, semctl, DEFAULT_VERSION);
More information about the Libc-alpha
mailing list