| Message ID | 20200330102945.2388294-2-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/1] linux-user: Support futex_time64 | expand |
On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > Add support for host and target futex_time64. If futex_time64 exists on > the host we try that first before falling back to the standard futex > syscall. Hi; I dunno why Coverity's only just noticed this, but in CID 1432339 it points out: > +#if defined(TARGET_NR_futex_time64) > +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, > + target_ulong uaddr2, int val3) > +{ > + struct timespec ts, *pts; > + int base_op; > + > + /* ??? We assume FUTEX_* constants are the same on both host > + and target. */ > +#ifdef FUTEX_CMD_MASK > + base_op = op & FUTEX_CMD_MASK; > +#else > + base_op = op; > +#endif > + switch (base_op) { > + case FUTEX_WAIT: > + case FUTEX_WAIT_BITSET: > + if (timeout) { > + pts = &ts; > + target_to_host_timespec64(pts, timeout); ...that here we call target_to_host_timespec64(), which can fail with -TARGET_EFAULT, but (unlike all the other times we call the function) we aren't checking its return value. Is there missing error handling code here ? > + } else { > + pts = NULL; > + } thanks -- PMM
Le 02/11/2020 à 19:15, Peter Maydell a écrit : > On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote: >> >> From: Alistair Francis <alistair.francis@wdc.com> >> >> Add support for host and target futex_time64. If futex_time64 exists on >> the host we try that first before falling back to the standard futex >> syscall. > > Hi; I dunno why Coverity's only just noticed this, but in > CID 1432339 it points out: > >> +#if defined(TARGET_NR_futex_time64) >> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, >> + target_ulong uaddr2, int val3) >> +{ >> + struct timespec ts, *pts; >> + int base_op; >> + >> + /* ??? We assume FUTEX_* constants are the same on both host >> + and target. */ >> +#ifdef FUTEX_CMD_MASK >> + base_op = op & FUTEX_CMD_MASK; >> +#else >> + base_op = op; >> +#endif >> + switch (base_op) { >> + case FUTEX_WAIT: >> + case FUTEX_WAIT_BITSET: >> + if (timeout) { >> + pts = &ts; >> + target_to_host_timespec64(pts, timeout); > > ...that here we call target_to_host_timespec64(), which can > fail with -TARGET_EFAULT, but (unlike all the other times we call > the function) we aren't checking its return value. > Is there missing error handling code here ? > I think the code is like that because this is a cut&paste of function do_futex() witl "s/timespec/timespec64/". And yes I think we should check for the return value. I'm going to fix that. Thanks, Laurent
On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote: > > Le 02/11/2020 à 19:15, Peter Maydell a écrit : > > On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote: > >> > >> From: Alistair Francis <alistair.francis@wdc.com> > >> > >> Add support for host and target futex_time64. If futex_time64 exists on > >> the host we try that first before falling back to the standard futex > >> syscall. > > > > Hi; I dunno why Coverity's only just noticed this, but in > > CID 1432339 it points out: > > > >> +#if defined(TARGET_NR_futex_time64) > >> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, > >> + target_ulong uaddr2, int val3) > >> +{ > >> + struct timespec ts, *pts; > >> + int base_op; > >> + > >> + /* ??? We assume FUTEX_* constants are the same on both host > >> + and target. */ > >> +#ifdef FUTEX_CMD_MASK > >> + base_op = op & FUTEX_CMD_MASK; > >> +#else > >> + base_op = op; > >> +#endif > >> + switch (base_op) { > >> + case FUTEX_WAIT: > >> + case FUTEX_WAIT_BITSET: > >> + if (timeout) { > >> + pts = &ts; > >> + target_to_host_timespec64(pts, timeout); > > > > ...that here we call target_to_host_timespec64(), which can > > fail with -TARGET_EFAULT, but (unlike all the other times we call > > the function) we aren't checking its return value. > > Is there missing error handling code here ? > > > > I think the code is like that because this is a cut&paste of function > do_futex() witl "s/timespec/timespec64/". > > And yes I think we should check for the return value. > I'm going to fix that. Thanks! Let me know if you want me to do it and I can send a patch instead. Alistair > > Thanks, > Laurent >
Le 03/11/2020 à 16:40, Alistair Francis a écrit : > On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote: >> >> Le 02/11/2020 à 19:15, Peter Maydell a écrit : >>> On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote: >>>> >>>> From: Alistair Francis <alistair.francis@wdc.com> >>>> >>>> Add support for host and target futex_time64. If futex_time64 exists on >>>> the host we try that first before falling back to the standard futex >>>> syscall. >>> >>> Hi; I dunno why Coverity's only just noticed this, but in >>> CID 1432339 it points out: >>> >>>> +#if defined(TARGET_NR_futex_time64) >>>> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, >>>> + target_ulong uaddr2, int val3) >>>> +{ >>>> + struct timespec ts, *pts; >>>> + int base_op; >>>> + >>>> + /* ??? We assume FUTEX_* constants are the same on both host >>>> + and target. */ >>>> +#ifdef FUTEX_CMD_MASK >>>> + base_op = op & FUTEX_CMD_MASK; >>>> +#else >>>> + base_op = op; >>>> +#endif >>>> + switch (base_op) { >>>> + case FUTEX_WAIT: >>>> + case FUTEX_WAIT_BITSET: >>>> + if (timeout) { >>>> + pts = &ts; >>>> + target_to_host_timespec64(pts, timeout); >>> >>> ...that here we call target_to_host_timespec64(), which can >>> fail with -TARGET_EFAULT, but (unlike all the other times we call >>> the function) we aren't checking its return value. >>> Is there missing error handling code here ? >>> >> >> I think the code is like that because this is a cut&paste of function >> do_futex() witl "s/timespec/timespec64/". >> >> And yes I think we should check for the return value. >> I'm going to fix that. > > Thanks! Let me know if you want me to do it and I can send a patch instead. > If you have time, please do. Thanks Laurent
On Tue, Nov 3, 2020 at 8:00 AM Laurent Vivier <laurent@vivier.eu> wrote: > > Le 03/11/2020 à 16:40, Alistair Francis a écrit : > > On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote: > >> > >> Le 02/11/2020 à 19:15, Peter Maydell a écrit : > >>> On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote: > >>>> > >>>> From: Alistair Francis <alistair.francis@wdc.com> > >>>> > >>>> Add support for host and target futex_time64. If futex_time64 exists on > >>>> the host we try that first before falling back to the standard futex > >>>> syscall. > >>> > >>> Hi; I dunno why Coverity's only just noticed this, but in > >>> CID 1432339 it points out: > >>> > >>>> +#if defined(TARGET_NR_futex_time64) > >>>> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, > >>>> + target_ulong uaddr2, int val3) > >>>> +{ > >>>> + struct timespec ts, *pts; > >>>> + int base_op; > >>>> + > >>>> + /* ??? We assume FUTEX_* constants are the same on both host > >>>> + and target. */ > >>>> +#ifdef FUTEX_CMD_MASK > >>>> + base_op = op & FUTEX_CMD_MASK; > >>>> +#else > >>>> + base_op = op; > >>>> +#endif > >>>> + switch (base_op) { > >>>> + case FUTEX_WAIT: > >>>> + case FUTEX_WAIT_BITSET: > >>>> + if (timeout) { > >>>> + pts = &ts; > >>>> + target_to_host_timespec64(pts, timeout); > >>> > >>> ...that here we call target_to_host_timespec64(), which can > >>> fail with -TARGET_EFAULT, but (unlike all the other times we call > >>> the function) we aren't checking its return value. > >>> Is there missing error handling code here ? > >>> > >> > >> I think the code is like that because this is a cut&paste of function > >> do_futex() witl "s/timespec/timespec64/". > >> > >> And yes I think we should check for the return value. > >> I'm going to fix that. > > > > Thanks! Let me know if you want me to do it and I can send a patch instead. > > > > If you have time, please do. Sending the patch now. Alistair > > Thanks > Laurent >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 49395dcea978..5af55fca7811 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -245,7 +245,12 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 arg4,type5 arg5, \ #define __NR_sys_rt_sigqueueinfo __NR_rt_sigqueueinfo #define __NR_sys_rt_tgsigqueueinfo __NR_rt_tgsigqueueinfo #define __NR_sys_syslog __NR_syslog -#define __NR_sys_futex __NR_futex +#if defined(__NR_futex) +# define __NR_sys_futex __NR_futex +#endif +#if defined(__NR_futex_time64) +# define __NR_sys_futex_time64 __NR_futex_time64 +#endif #define __NR_sys_inotify_init __NR_inotify_init #define __NR_sys_inotify_add_watch __NR_inotify_add_watch #define __NR_sys_inotify_rm_watch __NR_inotify_rm_watch @@ -295,10 +300,14 @@ _syscall1(int,exit_group,int,error_code) #if defined(TARGET_NR_set_tid_address) && defined(__NR_set_tid_address) _syscall1(int,set_tid_address,int *,tidptr) #endif -#if (defined(TARGET_NR_futex) || defined(TARGET_NR_exit)) && defined(__NR_futex) +#if defined(__NR_futex) _syscall6(int,sys_futex,int *,uaddr,int,op,int,val, const struct timespec *,timeout,int *,uaddr2,int,val3) #endif +#if defined(__NR_futex_time64) +_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val, + const struct timespec *,timeout,int *,uaddr2,int,val3) +#endif #define __NR_sys_sched_getaffinity __NR_sched_getaffinity _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len, unsigned long *, user_mask_ptr); @@ -762,10 +771,14 @@ safe_syscall5(int, ppoll, struct pollfd *, ufds, unsigned int, nfds, safe_syscall6(int, epoll_pwait, int, epfd, struct epoll_event *, events, int, maxevents, int, timeout, const sigset_t *, sigmask, size_t, sigsetsize) -#ifdef TARGET_NR_futex +#if defined(__NR_futex) safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \ const struct timespec *,timeout,int *,uaddr2,int,val3) #endif +#if defined(__NR_futex_time64) +safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \ + const struct timespec *,timeout,int *,uaddr2,int,val3) +#endif safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize) safe_syscall2(int, kill, pid_t, pid, int, sig) safe_syscall2(int, tkill, int, tid, int, sig) @@ -1229,7 +1242,7 @@ static inline abi_long target_to_host_timespec(struct timespec *host_ts, } #endif -#if defined(TARGET_NR_clock_settime64) +#if defined(TARGET_NR_clock_settime64) || defined(TARGET_NR_futex_time64) static inline abi_long target_to_host_timespec64(struct timespec *host_ts, abi_ulong target_addr) { @@ -6916,6 +6929,55 @@ static inline abi_long host_to_target_statx(struct target_statx *host_stx, } #endif +static int do_sys_futex(int *uaddr, int op, int val, + const struct timespec *timeout, int *uaddr2, + int val3) +{ +#if HOST_LONG_BITS == 64 +#if defined(__NR_futex) + /* always a 64-bit time_t, it doesn't define _time64 version */ + return sys_futex(uaddr, op, val, timeout, uaddr2, val3); + +#endif +#else /* HOST_LONG_BITS == 64 */ +#if defined(__NR_futex_time64) + if (sizeof(timeout->tv_sec) == 8) { + /* _time64 function on 32bit arch */ + return sys_futex_time64(uaddr, op, val, timeout, uaddr2, val3); + } +#endif +#if defined(__NR_futex) + /* old function on 32bit arch */ + return sys_futex(uaddr, op, val, timeout, uaddr2, val3); +#endif +#endif /* HOST_LONG_BITS == 64 */ + g_assert_not_reached(); +} + +static int do_safe_futex(int *uaddr, int op, int val, + const struct timespec *timeout, int *uaddr2, + int val3) +{ +#if HOST_LONG_BITS == 64 +#if defined(__NR_futex) + /* always a 64-bit time_t, it doesn't define _time64 version */ + return get_errno(safe_futex(uaddr, op, val, timeout, uaddr2, val3)); +#endif +#else /* HOST_LONG_BITS == 64 */ +#if defined(__NR_futex_time64) + if (sizeof(timeout->tv_sec) == 8) { + /* _time64 function on 32bit arch */ + return get_errno(safe_futex_time64(uaddr, op, val, timeout, uaddr2, + val3)); + } +#endif +#if defined(__NR_futex) + /* old function on 32bit arch */ + return get_errno(safe_futex(uaddr, op, val, timeout, uaddr2, val3)); +#endif +#endif /* HOST_LONG_BITS == 64 */ + return -TARGET_ENOSYS; +} /* ??? Using host futex calls even when target atomic operations are not really atomic probably breaks things. However implementing @@ -6945,12 +7007,11 @@ static int do_futex(target_ulong uaddr, int op, int val, target_ulong timeout, } else { pts = NULL; } - return get_errno(safe_futex(g2h(uaddr), op, tswap32(val), - pts, NULL, val3)); + return do_safe_futex(g2h(uaddr), op, tswap32(val), pts, NULL, val3); case FUTEX_WAKE: - return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0)); + return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0); case FUTEX_FD: - return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0)); + return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0); case FUTEX_REQUEUE: case FUTEX_CMP_REQUEUE: case FUTEX_WAKE_OP: @@ -6960,16 +7021,63 @@ static int do_futex(target_ulong uaddr, int op, int val, target_ulong timeout, to satisfy the compiler. We do not need to tswap TIMEOUT since it's not compared to guest memory. */ pts = (struct timespec *)(uintptr_t) timeout; - return get_errno(safe_futex(g2h(uaddr), op, val, pts, - g2h(uaddr2), - (base_op == FUTEX_CMP_REQUEUE - ? tswap32(val3) - : val3))); + return do_safe_futex(g2h(uaddr), op, val, pts, g2h(uaddr2), + (base_op == FUTEX_CMP_REQUEUE + ? tswap32(val3) + : val3)); default: return -TARGET_ENOSYS; } } #endif + +#if defined(TARGET_NR_futex_time64) +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout, + target_ulong uaddr2, int val3) +{ + struct timespec ts, *pts; + int base_op; + + /* ??? We assume FUTEX_* constants are the same on both host + and target. */ +#ifdef FUTEX_CMD_MASK + base_op = op & FUTEX_CMD_MASK; +#else + base_op = op; +#endif + switch (base_op) { + case FUTEX_WAIT: + case FUTEX_WAIT_BITSET: + if (timeout) { + pts = &ts; + target_to_host_timespec64(pts, timeout); + } else { + pts = NULL; + } + return do_safe_futex(g2h(uaddr), op, tswap32(val), pts, NULL, val3); + case FUTEX_WAKE: + return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0); + case FUTEX_FD: + return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0); + case FUTEX_REQUEUE: + case FUTEX_CMP_REQUEUE: + case FUTEX_WAKE_OP: + /* For FUTEX_REQUEUE, FUTEX_CMP_REQUEUE, and FUTEX_WAKE_OP, the + TIMEOUT parameter is interpreted as a uint32_t by the kernel. + But the prototype takes a `struct timespec *'; insert casts + to satisfy the compiler. We do not need to tswap TIMEOUT + since it's not compared to guest memory. */ + pts = (struct timespec *)(uintptr_t) timeout; + return do_safe_futex(g2h(uaddr), op, val, pts, g2h(uaddr2), + (base_op == FUTEX_CMP_REQUEUE + ? tswap32(val3) + : val3)); + default: + return -TARGET_ENOSYS; + } +} +#endif + #if defined(TARGET_NR_name_to_handle_at) && defined(CONFIG_OPEN_BY_HANDLE) static abi_long do_name_to_handle_at(abi_long dirfd, abi_long pathname, abi_long handle, abi_long mount_id, @@ -7541,7 +7649,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, ts = cpu->opaque; if (ts->child_tidptr) { put_user_u32(0, ts->child_tidptr); - sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX, + do_sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX, NULL, NULL, 0); } thread_cpu = NULL; @@ -11635,6 +11743,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, case TARGET_NR_futex: return do_futex(arg1, arg2, arg3, arg4, arg5, arg6); #endif +#ifdef TARGET_NR_futex_time64 + case TARGET_NR_futex_time64: + return do_futex_time64(arg1, arg2, arg3, arg4, arg5, arg6); +#endif #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init) case TARGET_NR_inotify_init: ret = get_errno(sys_inotify_init());