[v4,3/3] sysv: linux: Pass 64-bit version of semctl syscall
Commit Message
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
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).
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
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.)
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).
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
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;
};
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;
> };
@@ -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
@@ -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
@@ -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
{
@@ -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
@@ -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
@@ -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
@@ -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
{