This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] sysv: linux: Pass 64-bit version of semctl syscall
On Fri, Feb 21, 2020 at 6:03 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 04/02/2020 21:56, 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 union semun to include a struct __semid_ds32
> > which supports 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.
> > ---
> > v2:
> > - Fix the HPPA error
> > - Simplfy the code changes
> >
> > include/sys/sem.h | 35 +++++++++++++++++++
> > sysdeps/unix/sysv/linux/bits/sem-pad.h | 1 +
> > sysdeps/unix/sysv/linux/hppa/bits/sem-pad.h | 2 ++
> > sysdeps/unix/sysv/linux/mips/bits/sem-pad.h | 2 ++
> > .../unix/sysv/linux/powerpc/bits/sem-pad.h | 2 ++
> > sysdeps/unix/sysv/linux/semctl.c | 23 +++++++++---
> > sysdeps/unix/sysv/linux/sparc/bits/sem-pad.h | 2 ++
> > sysdeps/unix/sysv/linux/x86/bits/sem-pad.h | 1 +
> > 8 files changed, 64 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/sys/sem.h b/include/sys/sem.h
> > index 69fdf1f752..70b83127cb 100644
> > --- a/include/sys/sem.h
> > +++ b/include/sys/sem.h
> > @@ -5,5 +5,40 @@
> >
> > __typeof__ (semtimedop) __semtimedop attribute_hidden;
> >
> > +# endif
> > +
> > +# ifdef __SEMID_DS_HIGH
> > +# if defined (__SEMID_DS_HIGH_END)
>
> The current idea for per-architecture overrides is to define the expected
> default value on the generic header and check for the value *value* instead
> of *existence*. So here it should be:
>
> #if __SEMID_DS_HIGH
> # if __SEMID_DS_HIGH_END
> ...
> # elif __SEMID_DS_HIGH_SWAP
> ...
> # else
> # endif
> #endif
>
> So you need to define __SEMID_DS_HIGH, __SEMID_DS_HIGH_END, and
> __SEMID_DS_HIGH_SWAP for all architectures (this is how __SEM_PAD_BEFORE_TIME
> and __SEM_PAD_AFTER_TIME are organized).
>
> And I think it might be worth to add a _Static_assert to avoid possible
> errors if an architecture wrongly set __SEMID_DS_HIGH_END and __SEMID_DS_HIGH_SWAP:
>
> #if __SEMID_DS_HIGH
> # if __SEMID_DS_HIGH_END
> _Static_assert (!__SEMID_DS_HIGH_SWAP, "__SEMID_DS_HIGH_SWAP if defined");
> ...
> # elif __SEMID_DS_HIGH_SWAP
> ...
> # else
> # endif
> #endif
I am dropping all of the defines based on your comments further down.
>
> > +struct __semid_ds32 {
> > + struct ipc_perm sem_perm; /* permissions .. see ipc.h */
> > + __syscall_ulong_t sem_otime; /* last semop time */
> > + __syscall_ulong_t sem_ctime; /* last change time */
> > + __syscall_ulong_t sem_nsems; /* no. of semaphores in array */
> > + __syscall_ulong_t sem_otime_high;
> > + __syscall_ulong_t sem_ctime_high;
> > +};
> > +# elif defined (__SEMID_DS_HIGH_SWAP)
> > +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;
> > +};
> > +# else
> > +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
> > # endif
> > #endif
>
> From my pthread refactoring, I wonder if would be better to parametriz the
> semid_ds instead and define the sem_{o,c}time{_time} directly on the structure
> instead of adding define layer.
>
> So we will have an installed Linux semid_ds_t.h header which Linux sem.h includes
> and each architecture redefines if required. It would allow to add the requires
> y2038 fields you are proposing and semctl will change it directly.
I have added a architecture specific semid_ds_t.h, I don't see how I
can use that for a y2038 safe implementation though, as we need to
expose to user space sem_{c, o}time as time_t (64-bit) but expose it
to the kernel as ulong (32-bit) sem_{c, o}time{_high}.
Without adding a new __semid_ds32I (for internal use) or creating a
internal use union semun I don't see how I could do this.
>
> > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > index 566ce039cc..18235e56df 100644
> > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > @@ -31,3 +31,4 @@
> >
> > #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32)
> > #define __SEM_PAD_BEFORE_TIME 0
> > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > diff --git a/sysdeps/unix/sysv/linux/hppa/bits/sem-pad.h b/sysdeps/unix/sysv/linux/hppa/bits/sem-pad.h
> > index ee0332325b..62580792b3 100644
> > --- a/sysdeps/unix/sysv/linux/hppa/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/hppa/bits/sem-pad.h
> > @@ -24,3 +24,5 @@
> >
> > #define __SEM_PAD_AFTER_TIME 0
> > #define __SEM_PAD_BEFORE_TIME (__TIMESIZE == 32)
> > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > +#define __SEMID_DS_HIGH_SWAP (__WORDSIZE == 32)
> > diff --git a/sysdeps/unix/sysv/linux/mips/bits/sem-pad.h b/sysdeps/unix/sysv/linux/mips/bits/sem-pad.h
> > index 4c581f7694..12f2bd9c62 100644
> > --- a/sysdeps/unix/sysv/linux/mips/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/mips/bits/sem-pad.h
> > @@ -22,3 +22,5 @@
> >
> > #define __SEM_PAD_AFTER_TIME 0
> > #define __SEM_PAD_BEFORE_TIME 0
> > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > +#define __SEMID_DS_HIGH_END (__WORDSIZE == 32)
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/sem-pad.h b/sysdeps/unix/sysv/linux/powerpc/bits/sem-pad.h
> > index 42d8827906..a13ca5f6bd 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/powerpc/bits/sem-pad.h
> > @@ -24,3 +24,5 @@
> >
> > #define __SEM_PAD_AFTER_TIME 0
> > #define __SEM_PAD_BEFORE_TIME (__TIMESIZE == 32)
> > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > +#define __SEMID_DS_HIGH_SWAP (__WORDSIZE == 32)
> > diff --git a/sysdeps/unix/sysv/linux/semctl.c b/sysdeps/unix/sysv/linux/semctl.c
> > index 0c3eb0932f..8dcb012b0b 100644
> > --- a/sysdeps/unix/sysv/linux/semctl.c
> > +++ b/sysdeps/unix/sysv/linux/semctl.c
> > @@ -28,6 +28,9 @@ union semun
> > {
> > int val; /* value for SETVAL */
> > struct semid_ds *buf; /* buffer for IPC_STAT & IPC_SET */
> > +#ifdef __SEMID_DS_HIGH
> > + struct __semid_ds32 *buf32; /* buffer for IPC_STAT & IPC_SET on 32-bit systems */
> > +#endif
> > unsigned short int *array; /* array for GETALL & SETALL */
> > struct seminfo *__buf; /* buffer for IPC_INFO */
> > };
> > @@ -43,13 +46,25 @@ 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
> > +#ifdef __SEMID_DS_HIGH
> > +/* If we aren't going to overflow the time_t sem_ctime/sem_otime set it */
> > +# if __TIMESIZE == 64 && __WORDSIZE == 32
>
> My understanding is for the SysV IPC y2038-safe call it should be done with
> a different IPC_STAT value than current one (with the __IPC_64 bit set).
> So I think a better strategy would be to:
>
> 1. Check if IPC_STAT has the __IPC_64 bit set.
> 2. Only set the y2038 safe values if command also has __IPC_64 bit set.
> 3. Adjust all the requires architecture to have IPC_STAT with __IPC_64 set.
> 4. For time32 symbol, clear the __IPC_64 bit.
>
> So here it would be:
>
> [...]
> #define __IPC_TIME64 (IPC_STAT & __IPC_64)
> [...]
>
> #if __IPC_TIME64
> if (ret == 0 && (cmd & __IPC_TIME64))
> {
> [...]
> }
> #endif
>
> It wouldn't be active since IPC_STAT does not have IPC_64 set currently.
I think I have addressed this.
>
> > + if (ret == 0)
> > + {
> > + arg.buf->sem_ctime = arg.buf32->sem_ctime | ((time_t) arg.buf32->sem_ctime_high << 32);
> > + arg.buf->sem_otime = arg.buf32->sem_otime | ((time_t) arg.buf32->sem_otime_high << 32);
> > + }
> > +# endif
>
> Line too long.
Fixed.
Alistair
>
> > +#endif
> > + return ret;
> > }
> >
> > int
> > diff --git a/sysdeps/unix/sysv/linux/sparc/bits/sem-pad.h b/sysdeps/unix/sysv/linux/sparc/bits/sem-pad.h
> > index 5f4e214d12..79655b8149 100644
> > --- a/sysdeps/unix/sysv/linux/sparc/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/sparc/bits/sem-pad.h
> > @@ -24,3 +24,5 @@
> >
> > #define __SEM_PAD_AFTER_TIME 0
> > #define __SEM_PAD_BEFORE_TIME (__TIMESIZE == 32)
> > +#define __SEMID_DS_HIGH (__WORDSIZE == 32)
> > +#define __SEMID_DS_HIGH_SWAP (__WORDSIZE == 32)
> > diff --git a/sysdeps/unix/sysv/linux/x86/bits/sem-pad.h b/sysdeps/unix/sysv/linux/x86/bits/sem-pad.h
> > index 102e226997..db397857e7 100644
> > --- a/sysdeps/unix/sysv/linux/x86/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/x86/bits/sem-pad.h
> > @@ -22,3 +22,4 @@
> >
> > #define __SEM_PAD_AFTER_TIME 1
> > #define __SEM_PAD_BEFORE_TIME 0
> > +#define __SEMID_DS_HIGH 1
> >