Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211021055408.4006408-1-alistair.francis@opensource.wdc.com>
In-Reply-To: <20211021055408.4006408-1-alistair.francis@opensource.wdc.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 21 Oct 2021 08:30:56 +0200
Message-ID: <CAK8P3a3t8TLiXtp38=nYanwJyDNtX4=ANU8FXx8cr5w-W3hPRQ@mail.gmail.com>
Subject: Re: [PATCH v2] uapi: futex: Add a futex syscall
To:     Alistair Francis <alistair.francis@opensource.wdc.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alistair Francis <alistair23@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Alistair Francis <alistair.francis@wdc.com>,
        Linux API <linux-api@vger.kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Darren Hart <dvhart@infradead.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        =?UTF-8?Q?Andr=C3=A9_Almeida?= <andrealmeid@collabora.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 21, 2021 at 7:54 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> This commit adds two futex syscall wrappers that are exposed to
> userspace.
>
> Neither the kernel or glibc currently expose a futex wrapper, so
> userspace is left performing raw syscalls. This has mostly been becuase
> the overloading of one of the arguments makes it impossible to provide a
> single type safe function.
>
> Until recently the single syscall has worked fine. With the introduction
> of a 64-bit time_t futex call on 32-bit architectures, this has become
> more complex. The logic of handling the two possible futex syscalls is
> complex and often implemented incorrectly.
>
> This patch adds two futux syscall functions that correctly handle the
> time_t complexity for userspace.
>
> This idea is based on previous discussions: https://lkml.org/lkml/2021/9/21/143
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

This looks good to me, it addresses my earlier feedback, but I think we
need others to look into the question of whether we want this to be a
single function (as I suggested last time) or a pair of them (as you did).

I just replied to your email about this at
https://lore.kernel.org/lkml/CAK8P3a1CxFfHze6id1sQbQXV-x8DXkEdfqh51MwabzwhKAoTdQ@mail.gmail.com/

I added the futex maintainers and the linux-api list to Cc for them to
reply. Full patch quoted below, no further comments from me.

        Arnd

> ---
>  include/uapi/linux/futex_syscall.h | 81 ++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 include/uapi/linux/futex_syscall.h
>
> diff --git a/include/uapi/linux/futex_syscall.h b/include/uapi/linux/futex_syscall.h
> new file mode 100644
> index 0000000000000..f84a0c68baf78
> --- /dev/null
> +++ b/include/uapi/linux/futex_syscall.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_FUTEX_SYSCALL_H
> +#define _UAPI_LINUX_FUTEX_SYSCALL_H
> +
> +#include <asm/unistd.h>
> +#include <errno.h>
> +#include <linux/types.h>
> +#include <linux/time_types.h>
> +#include <sys/syscall.h>
> +
> +/**
> + * futex_syscall_timeout() - __NR_futex/__NR_futex_time64 syscall wrapper
> + * @uaddr:  address of first futex
> + * @op:   futex op code
> + * @val:  typically expected value of uaddr, but varies by op
> + * @timeout:  an absolute struct timespec
> + * @uaddr2: address of second futex for some ops
> + * @val3: varies by op
> + */
> +static inline int
> +__kernel_futex_syscall_timeout(volatile u_int32_t *uaddr, int op, u_int32_t val,
> +                     struct timespec *timeout, volatile u_int32_t *uaddr2, int val3)
> +{
> +#if defined(__NR_futex_time64)
> +       if (sizeof(*timeout) != sizeof(struct __kernel_old_timespec)) {
> +               int ret =  syscall(__NR_futex_time64, uaddr, op, val, timeout, uaddr2, val3);
> +
> +               if (ret == 0 || errno != ENOSYS)
> +                       return ret;
> +       }
> +#endif
> +
> +#if defined(__NR_futex)
> +       if (sizeof(*timeout) == sizeof(struct __kernel_old_timespec))
> +               return syscall(__NR_futex, uaddr, op, val, timeout, uaddr2, val3);
> +
> +       if (timeout && timeout->tv_sec == (long)timeout->tv_sec) {
> +               struct __kernel_old_timespec ts32;
> +
> +               ts32.tv_sec = (__kernel_long_t) timeout->tv_sec;
> +               ts32.tv_nsec = (__kernel_long_t) timeout->tv_nsec;
> +
> +               return syscall(__NR_futex, uaddr, op, val, &ts32, uaddr2, val3);
> +       } else if (!timeout) {
> +               return syscall(__NR_futex, uaddr, op, val, NULL, uaddr2, val3);
> +       }
> +#endif
> +
> +       errno = ENOSYS;
> +       return -1;
> +}
> +
> +/**
> + * futex_syscall_nr_requeue() - __NR_futex/__NR_futex_time64 syscall wrapper
> + * @uaddr:  address of first futex
> + * @op:   futex op code
> + * @val:  typically expected value of uaddr, but varies by op
> + * @nr_requeue:  an op specific meaning
> + * @uaddr2: address of second futex for some ops
> + * @val3: varies by op
> + */
> +static inline int
> +__kernel_futex_syscall_nr_requeue(volatile u_int32_t *uaddr, int op, u_int32_t val,
> +                        u_int32_t nr_requeue, volatile u_int32_t *uaddr2, int val3)
> +{
> +#if defined(__NR_futex_time64)
> +       int ret =  syscall(__NR_futex_time64, uaddr, op, val, nr_requeue, uaddr2, val3);
> +
> +       if (ret == 0 || errno != ENOSYS)
> +               return ret;
> +#endif
> +
> +#if defined(__NR_futex)
> +       return syscall(__NR_futex, uaddr, op, val, nr_requeue, uaddr2, val3);
> +#endif
> +
> +       errno = ENOSYS;
> +       return -1;
> +}
> +
> +#endif /* _UAPI_LINUX_FUTEX_SYSCALL_H */
> --
> 2.31.1
>