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

[Stepan Golosunov]

27.03.2020 в 12:45:23 -0700 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.

The ususal condition for 32-bit architectures without x32 seems to be

#if (__WORDSIZE == 32 \
     && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))


> >
> > > +  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.

But if there is padding before sem_otime added doesn't assignment to
arg.buf->sem_otime clobber arg.buf32->sem_ctime?

(And the situation will be even more complex when implementing y2038
support for older architectures like mips.)

This also seems to violate strict aliasing rules.

I guess it could be solved with something like

arg.buf->s = (struct semid_ds) {
  .sem_perm = arg.buf->s32.sem_perm,
  .sem_nsems = arg.buf->s32.sem_nsems,
  .sem_otime = arg.buf->s32.sem_otime | …
  …
}


> >
> > > +#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

I think the condition should be changed to

#ifdef __i386__

x32 does not need __semid_ds32.  If one was needed I guess it would
look like

struct __semid_ds32
{
  struct ipc_perm sem_perm;
  uint32_t  __glibc_reserved0;
  uint32_t sem_otime;
  int32_t sem_otime_high;
  __syscall_ulong_t __glibc_reserved1;
  uint32_t sem_ctime;
  int32_t sem_ctime_high;
  __syscall_ulong_t __glibc_reserved2;
  __syscall_ulong_t sem_nsems;
  __syscall_ulong_t __glibc_reserved3;
  __syscall_ulong_t __glibc_reserved4;
};