From: Alistair Francis <alistair23@gmail.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GNU C Library <libc-alpha@sourceware.org>,
Alistair Francis <alistair.francis@wdc.com>,
Arnd Bergmann <arnd.bergmann@linaro.org>
Subject: Re: [PATCH v6 3/3] sysv: linux: Pass 64-bit version of semctl syscall
Date: Wed, 29 Apr 2020 16:24:36 -0700 [thread overview]
Message-ID: <CAKmqyKNb-6SR4GqtQBimdUSxPOJLaUN-E3-Z-WWigCFYrEt51w@mail.gmail.com> (raw)
In-Reply-To: <3bec18bd-cbee-5853-2d36-df377b6cf7cf@linaro.org>
On Wed, Apr 29, 2020 at 2:09 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 28/04/2020 19:05, Alistair Francis via Libc-alpha 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.
> >
> > Due to alignment differences between 64-bit and 32-bit variables we
> > also need to set nsems to ensure it's correct.
>
> I think this approach might trigger an issue on semid_ds definition for
> time64 that I only noticed now.
>
> Since the idea of semctl is not use a different entrypoint neither to add
> aditional flag when issuing the syscall (either semctl or ipc), with this
> change we need to assure that __TIMESIZE == 32 sem_otime alignment matches
> __TIMESIZE == 64 one.
I don't follow. We don't assume that sem_otime has the same allignment
for __TIMESIZE == 32 and 64. In fact for RV32 they are not alligned
the same.
>
> However for semid_ds and msqid_ds time_t members can't not properly aligned
> to be expanded to 64-bit (shmid_ds has a shm_segsz field before the
> timestamp fields) due to the ipc_perm header consisting of 9 32-bit words
> except on parisc/powerpc/sparc. The alignment difference will make
> semctl once programs states build against a semid_ds with time_t
> being an 64-bit type.
I am missing something here as well, as that is exactly what RV32 is
testing and the glibc tests are passing (and a LInux rootFS boots).
>
> So we have 2 options here:
>
> 1. Keep the semid_ds layout as the kernel expected one and add the
> newer time64 timestamps at the *end*, and redefine the command
> arguments that passes a struct semid_ds to use
> (IPC_STAT, SEM_STAT, SEM_STAT_ANY) to indicate that the semid_ds
> is a time64 one. The IPC_64 bit might be a feasible hack since
> kernel won't use on possible new commands.
>
> 2. Add a new semctl64 to handle the time64 one semid_ds.
>
> > ---
> > .../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 | 47 +++++++++++++++++--
> > .../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, 233 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/hppa/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/hppa/struct__semid_ds32.h
> > new file mode 100644
> > index 0000000000..9eceaaff2d
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/hppa/struct__semid_ds32.h
> > @@ -0,0 +1,32 @@
> > +/* HPPA 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/>. */
> > +
> > +#if __WORDSIZE == 32
> > +/* 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_high; /* last semop() time high */
> > + __syscall_ulong_t sem_otime; /* last semop() time */
> > + __syscall_ulong_t sem_ctime_high; /* last time changed by semctl() high */
> > + __syscall_ulong_t sem_ctime; /* last time changed by semctl() */
> > + __syscall_ulong_t sem_nsems; /* number of semaphores in set */
> > + __syscall_ulong_t __glibc_reserved3;
> > + __syscall_ulong_t __glibc_reserved4;
> > +};
> > +#endif
>
> No need to define it for __WORDSIZE == 32, the ABI just supports it
> anyway.
Ok, I have removed these.
>
> > diff --git a/sysdeps/unix/sysv/linux/mips/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/mips/struct__semid_ds32.h
> > new file mode 100644
> > index 0000000000..2e3e2fc562
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/mips/struct__semid_ds32.h
> > @@ -0,0 +1,30 @@
> > +/* MIPS 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/>. */
> > +
> > +#if __WORDSIZE == 32
> > +/* 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_ctime; /* last change time */
> > + __syscall_ulong_t sem_nsems; /* number of semaphores in set */
> > + __syscall_ulong_t sem_otime_high;
> > + __syscall_ulong_t sem_ctime_high;
> > +};
> > +#endif
>
> Ok, it cover mips64-n32 as well.
>
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/powerpc/struct__semid_ds32.h
> > new file mode 100644
> > index 0000000000..f509db5a02
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/powerpc/struct__semid_ds32.h
> > @@ -0,0 +1,32 @@
> > +/* PowerPC 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/>. */
> > +
> > +#if __WORDSIZE == 32
> > +/* 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_high; /* last semop() time high */
> > + __syscall_ulong_t sem_otime; /* last semop() time */
> > + __syscall_ulong_t sem_ctime_high; /* last time changed by semctl() high */
> > + __syscall_ulong_t sem_ctime; /* last time changed by semctl() */
> > + __syscall_ulong_t sem_nsems; /* number of semaphores in set */
> > + __syscall_ulong_t __glibc_reserved3;
> > + __syscall_ulong_t __glibc_reserved4;
> > +};
> > +#endif
>
> Ok.
>
> > diff --git a/sysdeps/unix/sysv/linux/semctl.c b/sysdeps/unix/sysv/linux/semctl.c
> > index 30571af49f..0634cc1abe 100644
> > --- a/sysdeps/unix/sysv/linux/semctl.c
> > +++ b/sysdeps/unix/sysv/linux/semctl.c
> > @@ -22,8 +22,13 @@
> > #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 IPC_TIME64 \
> > + (__WORDSIZE == 32 && __TIMESIZE == 64 \
> > + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
>
> I think it would be better to move this definition to the common
> ipc_priv.h header since it will most likely be used on msgctl
> and shmctl y2038 support as well.
Done.
>
> > +
> > /* Define a `union semun' suitable for Linux here. */
> > union semun
> > {
> > @@ -44,13 +49,47 @@ union semun
> > static int
> > semctl_syscall (int semid, int semnum, int cmd, union semun arg)
> > {
> > + int ret;
> > +#if IPC_TIME64
>
> I think we shoul add a comment stating why we need a auxiliary buffer
> for STAT/STAT_ANY operations. Something like:
>
> A temporary buffer is used to avoid both an issue where the export
> semid_ds might not have follow kernel expected layout (due
> sem_{o,c}time alignment in 64-bit time case) and the issue where
> some kernel version might not clear the high bits when returning
> then sem_{o,c}time information.
Added
>
> > + 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_ctime = arg.buf->sem_ctime,
> > + .sem_nsems = arg.buf->sem_nsems,
> > + };
>
> We need to fully initialize the temporary struct to avoid a kernel
> issue fixed in 4693916846269d633a3664586650dbfac2c5562f (4.14) where
> kernel does not clear the timestamps high bits.
Added.
>
> > + orig = arg.buf;
> > + arg.buf = (struct semid_ds*)&tmp;
>
> Space before &tmp.
Added.
>
> > + 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
>
> Ok.
>
> > +
> > +#if IPC_TIME64
> > + if (ret >= 0 && restore)
> > + {
> > + 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;
>
> I think it would be better to use __time64_t explicit here.
Done.
Alistair
>
> > + }
> > +#endif
> > +
> > + return ret;
> > }
> >
> > int
> > diff --git a/sysdeps/unix/sysv/linux/sparc/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/sparc/struct__semid_ds32.h
> > new file mode 100644
> > index 0000000000..f641825d37
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/sparc/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/>. */
> > +
> > +#if __WORDSIZE == 32
> > +/* 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_high; /* last semop() time high */
> > + __syscall_ulong_t sem_otime; /* last semop() time */
> > + __syscall_ulong_t sem_ctime_high; /* last time changed by semctl() high */
> > + __syscall_ulong_t sem_ctime; /* last time changed by semctl() */
> > + __syscall_ulong_t sem_nsems; /* number of semaphores in set */
> > + __syscall_ulong_t __glibc_reserved3;
> > + __syscall_ulong_t __glibc_reserved4;
> > +};
> > +#endif
>
> Ok.
>
> > diff --git a/sysdeps/unix/sysv/linux/struct__semid_ds32.h b/sysdeps/unix/sysv/linux/struct__semid_ds32.h
> > new file mode 100644
> > index 0000000000..51ff83206d
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/struct__semid_ds32.h
> > @@ -0,0 +1,32 @@
> > +/* Generic 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/>. */
> > +
> > +#if __WORDSIZE == 32
> > +/* 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
>
> Ok.
>
> > 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..f65c9fc877
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86/struct__semid_ds32.h
> > @@ -0,0 +1,32 @@
> > +/* x86 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
> >
>
> Ok.
>
>
next prev parent reply other threads:[~2020-04-29 23:33 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-28 22:05 [PATCH v6 0/3] Support y2038 semctl_syscall() Alistair Francis
2020-04-28 22:05 ` [PATCH v6 1/3] bits/sem.h: Split out struct semid_ds Alistair Francis
2020-04-28 22:05 ` [PATCH v6 2/3] semctl: Remove the sem-pad.h file Alistair Francis
2020-04-28 22:05 ` [PATCH v6 3/3] sysv: linux: Pass 64-bit version of semctl syscall Alistair Francis
2020-04-29 21:09 ` Adhemerval Zanella
2020-04-29 23:24 ` Alistair Francis [this message]
2020-04-30 15:06 ` Adhemerval Zanella
2020-04-30 21:25 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKNb-6SR4GqtQBimdUSxPOJLaUN-E3-Z-WWigCFYrEt51w@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=adhemerval.zanella@linaro.org \
--cc=alistair.francis@wdc.com \
--cc=arnd.bergmann@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).