[PATCH v4 3/3] sysv: linux: Pass 64-bit version of semctl syscall

[Alistair Francis]

On Fri, Mar 27, 2020 at 11:25 AM Stepan Golosunov
<stepan@golosunov.pp.ru> wrote:
>
> 26.03.2020 в 09:37:24 -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.
> > ---
> >  sysdeps/unix/sysv/linux/bits/semid_ds_t.h     | 15 ++++++++++++
> >  .../unix/sysv/linux/hppa/bits/semid_ds_t.h    | 15 ++++++++++++
> >  .../unix/sysv/linux/mips/bits/semid_ds_t.h    | 13 ++++++++++
> >  .../unix/sysv/linux/powerpc/bits/semid_ds_t.h | 15 ++++++++++++
> >  sysdeps/unix/sysv/linux/semctl.c              | 24 +++++++++++++++----
> >  .../unix/sysv/linux/sparc/bits/semid_ds_t.h   | 15 ++++++++++++
> >  sysdeps/unix/sysv/linux/x86/bits/semid_ds_t.h | 15 ++++++++++++
> >  7 files changed, 108 insertions(+), 4 deletions(-)
> >
>
> > --- a/sysdeps/unix/sysv/linux/semctl.c
> > +++ b/sysdeps/unix/sysv/linux/semctl.c
> > @@ -29,6 +29,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
> >    unsigned short int *array; /* array for GETALL & SETALL */
> >    struct seminfo *__buf;     /* buffer for IPC_INFO */
> >  };
> > @@ -44,13 +47,26 @@ 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
>
> This condition is wrong for x32 unless someone invents __semid_ds32
> with 32-bit time fields there.

Ah, good point. I guess there needs to be a && !defined(__ILP32__)
added to this.

>
> > +  if (ret == 0 && cmd == IPC_STAT)
>
> Is cmd == IPC_STAT the only case when conversion is needed?
> Brief glance at kernel sources suggests that SEM_STAT and SEM_STAT_ANY
> are also relevant.

I can double check.

>
> > +    {
> > +      arg.buf->sem_nsems = arg.buf32->sem_nsems;
> > +      arg.buf->sem_otime = arg.buf32->sem_otime |
> > +                               ((time_t) arg.buf32->sem_otime_high << 32);
> > +      arg.buf->sem_ctime = arg.buf32->sem_ctime |
> > +                               ((time_t) arg.buf32->sem_ctime_high << 32);
> > +    }
>
> Note that this is supposed to do nothing on little-endian asm-generic
> architectures (thus hiding bugs for big-endian ones).
> It's also not obvious why it attempts to copy sem_nsems (if sem_nsems
> ever changes it's location the assignment will likely clobber some of the
> sem{o,c}time{,_high}).

This is somewhat explained in the commit message.

For RV32 (and probably others) due to alignment constraints being
different between 32 and 64-bit types sem_nsems is actually at a
different offset in bug32 then buf. The kernel fills it out at the
alignment of buf32 so we need to move it to the alignment of buf.

The same issues applies to both types, so this isn't just for
big-endian machines.

>
> > +#endif
> > +  return ret;
> >  }
> >
> >  int
>
> > --- a/sysdeps/unix/sysv/linux/x86/bits/semid_ds_t.h
> > +++ b/sysdeps/unix/sysv/linux/x86/bits/semid_ds_t.h
> > @@ -20,6 +20,21 @@
> >  # error "Never include <bits/semid_ds_t.h> directly; use <sys/sem.h> instead."
> >  #endif
> >
> > +#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
>
> This is wrong for x32 but might work by accident (as long as kernel
> does not use its __unused1 and __unused2 fields).

Do you know what it should be?

I see this struct in: arch/x86/include/uapi/asm/sembuf.h

Alistair