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: <20210709001328.329716-1-andrealmeid@collabora.com> <20210709001328.329716-3-andrealmeid@collabora.com>
In-Reply-To: <20210709001328.329716-3-andrealmeid@collabora.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Tue, 17 Aug 2021 10:50:57 +0200
Message-ID: <CAK8P3a0MO1qJLRkCH8KrZ3+=L66KOsMRmcbrUvYdMoKykdKoyQ@mail.gmail.com>
Subject: Re: [PATCH v5 02/11] futex2: Implement vectorized wait
To:     =?UTF-8?Q?Andr=C3=A9_Almeida?= <andrealmeid@collabora.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Darren Hart <dvhart@infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Collabora kernel ML <kernel@collabora.com>,
        Gabriel Krisman Bertazi <krisman@collabora.com>,
        pgriffais@valvesoftware.com, z.figura12@gmail.com,
        Joel Fernandes <joel@joelfernandes.org>,
        malteskarupke@fastmail.fm, Linux API <linux-api@vger.kernel.org>,
        Florian Weimer <fweimer@redhat.com>,
        GNU C Library <libc-alpha@sourceware.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Peter Oskolkov <posk@posk.io>,
        Andrey Semashev <andrey.semashev@gmail.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Nicholas Piggin <npiggin@gmail.com>,
        Adhemerval Zanella <adhemerval.zanella@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Jul 9, 2021 at 2:13 AM André Almeida <andrealmeid@collabora.com> wrote:
>
> Add support to wait on multiple futexes. This is the interface
> implemented by this syscall:
>
> futex_waitv(struct futex_waitv *waiters, unsigned int nr_futexes,
>             unsigned int flags, struct timespec *timo)
>
> struct futex_waitv {
>         __u64 val;
>         void *uaddr;
>         unsigned int flags;
> };

You should generally try to avoid structures with implicit padding
like this one.

>  arch/x86/entry/syscalls/syscall_32.tbl |   1 +
>  arch/x86/entry/syscalls/syscall_64.tbl |   1 +
>  include/linux/compat.h                 |   9 +
>  include/linux/futex.h                  | 108 ++++++--
>  include/uapi/asm-generic/unistd.h      |   4 +-

I would split out the syscall table changes from the implementation, but then
do the table changes for all architectures, at least when you get to a version
that gets close to being accepted.

> +#ifdef CONFIG_COMPAT
> +/**
> + * compat_futex_parse_waitv - Parse a waitv array from userspace
> + * @futexv:    Kernel side list of waiters to be filled
> + * @uwaitv:     Userspace list to be parsed
> + * @nr_futexes: Length of futexv
> + *
> + * Return: Error code on failure, pointer to a prepared futexv otherwise
> + */
> +static int compat_futex_parse_waitv(struct futex_vector *futexv,
> +                                   struct compat_futex_waitv __user *uwaitv,
> +                                   unsigned int nr_futexes)
> +{
> +       struct compat_futex_waitv aux;
> +       unsigned int i;
> +
> +       for (i = 0; i < nr_futexes; i++) {
> +               if (copy_from_user(&aux, &uwaitv[i], sizeof(aux)))
> +                       return -EFAULT;
> +
> +               if ((aux.flags & ~FUTEXV_WAITER_MASK) ||
> +                   (aux.flags & FUTEX_SIZE_MASK) != FUTEX_32)
> +                       return -EINVAL;
> +
> +               futexv[i].w.flags = aux.flags;
> +               futexv[i].w.val = aux.val;
> +               futexv[i].w.uaddr = compat_ptr(aux.uaddr);
> +               futexv[i].q = futex_q_init;
> +       }
> +
> +       return 0;
> +}
> +
> +COMPAT_SYSCALL_DEFINE4(futex_waitv, struct compat_futex_waitv __user *, waiters,
> +                      unsigned int, nr_futexes, unsigned int, flags,
> +                      struct __kernel_timespec __user *, timo)
> +{
> +       struct hrtimer_sleeper to;
> +       struct futex_vector *futexv;
> +       struct timespec64 ts;
> +       ktime_t time;
> +       int ret;

It would be nice to reduce the duplication a little. compat_sys_futex_waitv()
and sys_futex_waitv() only differ by a single line, in which they call
a different
parse function, and the two parse functions only differ in the layout of the
user space structure. The get_timespec64() call already has an
in_compat_syscall() check in it, so I would suggest having a single entry
point for native and compat mode, but either having the parse function
add another such check or making the structure layout compatible.

The normal way of doing this is to have a __u64 value instead of the pointer,
and then using u64_to_uptr() for the conversion. It might be nice to
add a global

typedef __u64 __kernel_uptr_t;

for this purpose.

       Arnd