| Message ID | 1e92bb4f2cc06e8f3adfbd9de653c8e93c0d288a.1561421042.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/20] y2038: Introduce internal for glibc struct __timespec64 | expand |
* Alistair Francis: > +/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */ > +#ifdef __NR_futex_time64 > +# define lll_futex_syscall(nargs, futexp, op, ...) \ > + ({ \ > + INTERNAL_SYSCALL_DECL (__err); \ > + long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \ > + __VA_ARGS__); \ > + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ > + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ > + }) > +#else > +# define lll_futex_syscall(nargs, futexp, op, ...) \ > + ({ \ > + INTERNAL_SYSCALL_DECL (__err); \ > + long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \ > + __VA_ARGS__); \ > + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ > + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ > + }) > +#endif I don't think a compile-time check in generic code is correct here. It will cause binaries to fail to run on older kernels which do not have the system call. This is obviously not a concern for 32-bit RISC-V, but it is not acceptable for i386, for example. I still think you should define __NR_futex in RV32 <sysdep.h>. Thanks, Florian
On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote: > * Alistair Francis: > >> +/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */ >> +#ifdef __NR_futex_time64 >> +# define lll_futex_syscall(nargs, futexp, op, ...) \ >> + ({ \ >> + INTERNAL_SYSCALL_DECL (__err); \ >> + long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \ >> + __VA_ARGS__); \ >> + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ >> + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ >> + }) >> +#else >> +# define lll_futex_syscall(nargs, futexp, op, ...) \ >> + ({ \ >> + INTERNAL_SYSCALL_DECL (__err); \ >> + long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \ >> + __VA_ARGS__); \ >> + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ >> + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ >> + }) >> +#endif > > I don't think a compile-time check in generic code is correct here. It > will cause binaries to fail to run on older kernels which do not have > the system call. This is obviously not a concern for 32-bit RISC-V, but > it is not acceptable for i386, for example. > > I still think you should define __NR_futex in RV32 <sysdep.h>. For existing 32-bit archs there needs to be a runtime check (if both __NR_futex and __NR_futex_time64 are defined). Andreas.
On Tue, Jun 25, 2019 at 1:26 PM Andreas Schwab <schwab@suse.de> wrote: > On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote: > > > > I don't think a compile-time check in generic code is correct here. It > > will cause binaries to fail to run on older kernels which do not have > > the system call. This is obviously not a concern for 32-bit RISC-V, but > > it is not acceptable for i386, for example. > > > > I still think you should define __NR_futex in RV32 <sysdep.h>. > > For existing 32-bit archs there needs to be a runtime check (if both > __NR_futex and __NR_futex_time64 are defined). That opens the question what type this function should take, because one of the two will require the time argument to be converted. For instance pthread_timedjoin_np() is a public interface that will need an additional version to deal with time64. Should pthread_timedjoin_np() convert the user timespec to __timespec64 and pass it down to lll_futex_syscall(), which then converts it back to timespec before calling __NR_futex() on 32-bit architectures, or is there a way to avoid the double conversion? Arnd Arnd
* Arnd Bergmann: > On Tue, Jun 25, 2019 at 1:26 PM Andreas Schwab <schwab@suse.de> wrote: >> On Jun 25 2019, Florian Weimer <fweimer@redhat.com> wrote: >> > >> > I don't think a compile-time check in generic code is correct here. It >> > will cause binaries to fail to run on older kernels which do not have >> > the system call. This is obviously not a concern for 32-bit RISC-V, but >> > it is not acceptable for i386, for example. >> > >> > I still think you should define __NR_futex in RV32 <sysdep.h>. >> >> For existing 32-bit archs there needs to be a runtime check (if both >> __NR_futex and __NR_futex_time64 are defined). > > That opens the question what type this function should take, > because one of the two will require the time argument to be > converted. > > For instance pthread_timedjoin_np() is a public interface > that will need an additional version to deal with time64. > Should pthread_timedjoin_np() convert the user timespec > to __timespec64 and pass it down to lll_futex_syscall(), > which then converts it back to timespec before calling > __NR_futex() on 32-bit architectures, or is there a way to > avoid the double conversion? My expectation is that glibc will not provide a 32-bit struct timespec for architectures which do not have a 32-bit __NR_futex system call. It's a 64-bit only world. Before the 2038 deadline, we will not support kernels that removed support for the 32-bit system calls. Based on that, I expect that the 32-bit struct timespec variant of pthread_timedjoin_np calls futex, and the 64-bit variant (probably named pthread_timedjoin64_np or something like that, or used by default under appropriate feature selection macros) will call futex_time64. This has the advantage that the type of call is visible at the syscall layer, and could eventually trigger kenrel warnings. With translation, that wouldn't be the case. It also increases compatibility with seccomp filters which know of the old system calls, but not the new ones. Thanks, Florian
On Tue, Jun 25, 2019 at 2:07 PM Florian Weimer <fweimer@redhat.com> wrote: > * Arnd Bergmann: > > For instance pthread_timedjoin_np() is a public interface > > that will need an additional version to deal with time64. > > Should pthread_timedjoin_np() convert the user timespec > > to __timespec64 and pass it down to lll_futex_syscall(), > > which then converts it back to timespec before calling > > __NR_futex() on 32-bit architectures, or is there a way to > > avoid the double conversion? > > My expectation is that glibc will not provide a 32-bit struct timespec > for architectures which do not have a 32-bit __NR_futex system call. > It's a 64-bit only world. Before the 2038 deadline, we will not support > kernels that removed support for the 32-bit system calls. > > Based on that, I expect that the 32-bit struct timespec variant of > pthread_timedjoin_np calls futex, and the 64-bit variant (probably named > pthread_timedjoin64_np or something like that, or used by default under > appropriate feature selection macros) will call futex_time64. This has > the advantage that the type of call is visible at the syscall layer, and > could eventually trigger kenrel warnings. With translation, that > wouldn't be the case. It also increases compatibility with seccomp > filters which know of the old system calls, but not the new ones. Ok, that's perfect as far as I'm concerned. From earlier discussions I had understood that you were planning to avoid the associated code duplication and only have one internal implementation for the two external interfaces. Arnd
diff --git a/ChangeLog b/ChangeLog index b90c5ab60c..a700783ef3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,7 @@ * nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of nanosleep. * sysdeps/unix/sysv/linux/nanosleep.c: Likewise. * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise. + * sysdeps/unix/sysv/linux/lowlevellock-futex.h: Use __NR_futex_time64 if we don't have __NR_futex. 2019-06-20 Dmitry V. Levin <ldv@altlinux.org> Florian Weimer <fweimer@redhat.com> diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h index 030a14b8dc..e310542bfa 100644 --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h @@ -65,14 +65,26 @@ (((fl) | FUTEX_PRIVATE_FLAG) ^ (private)) #endif -#define lll_futex_syscall(nargs, futexp, op, ...) \ - ({ \ - INTERNAL_SYSCALL_DECL (__err); \ - long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \ - __VA_ARGS__); \ - (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ - ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ - }) +/* __NR_futex isn't defined on all archs (RV32) so use __NR_futex_time64 */ +#ifdef __NR_futex_time64 +# define lll_futex_syscall(nargs, futexp, op, ...) \ + ({ \ + INTERNAL_SYSCALL_DECL (__err); \ + long int __ret = INTERNAL_SYSCALL (futex_time64, __err, nargs, futexp, op, \ + __VA_ARGS__); \ + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ + }) +#else +# define lll_futex_syscall(nargs, futexp, op, ...) \ + ({ \ + INTERNAL_SYSCALL_DECL (__err); \ + long int __ret = INTERNAL_SYSCALL (futex, __err, nargs, futexp, op, \ + __VA_ARGS__); \ + (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (__ret, __err)) \ + ? -INTERNAL_SYSCALL_ERRNO (__ret, __err) : 0); \ + }) +#endif #define lll_futex_wait(futexp, val, private) \ lll_futex_timed_wait (futexp, val, NULL, private)
The futex syscall isn't avaliable on newer 32-bit architectures (such as RV32) so if futex_time64 is avaliable lets use that instead. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- ChangeLog | 1 + sysdeps/unix/sysv/linux/lowlevellock-futex.h | 28 ++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-)