diff mbox series

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

Message ID 20200326163724.377351-4-alistair.francis@wdc.com
State New
Headers show
Series Support y2038 semctl_syscall() | expand

Commit Message

Alistair Francis March 26, 2020, 4:37 p.m. UTC
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(-)

Comments

Joseph Myers March 26, 2020, 7:01 p.m. UTC | #1
On Thu, 26 Mar 2020, Alistair Francis via Libc-alpha wrote:

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

As far as I can see, this type is not used in any public interface, just 
internally in the implementation.  Thus it should be defined in an 
internal header, not a bits/ installed header.

> +      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);

Split lines before not after operators, and make sure to include 
parentheses in such a case to ensure the operator at the start of the next 
line automatically goes in the correct column (see the GNU Coding 
Standards).
Alistair Francis March 26, 2020, 10:11 p.m. UTC | #2
On Thu, Mar 26, 2020 at 12:01 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 26 Mar 2020, Alistair Francis via Libc-alpha wrote:
>
> > +#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
>
> As far as I can see, this type is not used in any public interface, just
> internally in the implementation.  Thus it should be defined in an
> internal header, not a bits/ installed header.

It is only user internally.

The problem is I couldn't find a place to put it where architectures
can override it. Is there a place I'm missing where I could put it?

>
> > +      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);
>
> Split lines before not after operators, and make sure to include
> parentheses in such a case to ensure the operator at the start of the next
> line automatically goes in the correct column (see the GNU Coding
> Standards).

Fixed.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers March 26, 2020, 10:52 p.m. UTC | #3
On Thu, 26 Mar 2020, Alistair Francis via Libc-alpha wrote:

> The problem is I couldn't find a place to put it where architectures
> can override it. Is there a place I'm missing where I could put it?

Any .h or .c file in the glibc sources can be overridden for a particular 
configuration simply by putting a copy in a higher-priority sysdeps 
directory.  (But watch out for code using #include with "" instead of <>; 
if a .h or .c file is included like that, sysdeps overrides of the 
included file may not be effective.  In general, use <> not "" with 
#include in glibc to avoid that issue.)
Stepan Golosunov March 27, 2020, 6:24 p.m. UTC | #4
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.

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

> +    {
> +      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}).

> +#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).
Alistair Francis March 27, 2020, 7:45 p.m. UTC | #5
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
Stepan Golosunov March 28, 2020, 10:28 a.m. UTC | #6
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;
};
Alistair Francis April 1, 2020, 4:16 p.m. UTC | #7
On Sat, Mar 28, 2020 at 3:28 AM Stepan Golosunov <stepan@golosunov.pp.ru> wrote:
>
> 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))

That works, I have updated 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.

You are right, I am checking those other two conditions.

> >
> > >
> > > > +    {
> > > > +      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 | …
>   …
> }

Good idea, I have updated it to create a new struct.

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

I have changed the define and I don't define a struct __semid_ds32 for x32.


Alistair

>
> 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;
> };
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/bits/semid_ds_t.h
index d9d902ed0d..b135301356 100644
--- a/sysdeps/unix/sysv/linux/bits/semid_ds_t.h
+++ b/sysdeps/unix/sysv/linux/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
+
 /* Data structure describing a set of semaphores.  */
 #if __TIMESIZE == 32
 struct semid_ds
diff --git a/sysdeps/unix/sysv/linux/hppa/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/hppa/bits/semid_ds_t.h
index 39c0e53f38..3613c5ec94 100644
--- a/sysdeps/unix/sysv/linux/hppa/bits/semid_ds_t.h
+++ b/sysdeps/unix/sysv/linux/hppa/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_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
+
 /* Data structure describing a set of semaphores.  */
 #if __TIMESIZE == 32
 struct semid_ds
diff --git a/sysdeps/unix/sysv/linux/mips/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/mips/bits/semid_ds_t.h
index 1ab16492dd..e26906a67f 100644
--- a/sysdeps/unix/sysv/linux/mips/bits/semid_ds_t.h
+++ b/sysdeps/unix/sysv/linux/mips/bits/semid_ds_t.h
@@ -20,6 +20,19 @@ 
 # 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_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
+
 /* Data structure describing a set of semaphores.  */
 struct semid_ds
 {
diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/powerpc/bits/semid_ds_t.h
index 79b4cba939..ec2ff552eb 100644
--- a/sysdeps/unix/sysv/linux/powerpc/bits/semid_ds_t.h
+++ b/sysdeps/unix/sysv/linux/powerpc/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_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
+
 /* Data structure describing a set of semaphores.  */
 #if __TIMESIZE == 32
 struct semid_ds
diff --git a/sysdeps/unix/sysv/linux/semctl.c b/sysdeps/unix/sysv/linux/semctl.c
index 30571af49f..38c2fc6545 100644
--- 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
+  if (ret == 0 && cmd == IPC_STAT)
+    {
+      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);
+    }
+#endif
+  return ret;
 }
 
 int
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/sparc/bits/semid_ds_t.h
index f8de676e79..b08fb8a79e 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/semid_ds_t.h
+++ b/sysdeps/unix/sysv/linux/sparc/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_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
+
 /* Data structure describing a set of semaphores.  */
 #if __TIMESIZE == 32
 struct semid_ds
diff --git a/sysdeps/unix/sysv/linux/x86/bits/semid_ds_t.h b/sysdeps/unix/sysv/linux/x86/bits/semid_ds_t.h
index 42694069d5..c7b9adce88 100644
--- 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
+
 /* Data structure describing a set of semaphores.  */
 struct semid_ds
 {