linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: "André 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>
Subject: Re: [PATCH v5 02/11] futex2: Implement vectorized wait
Date: Tue, 17 Aug 2021 10:50:57 +0200	[thread overview]
Message-ID: <CAK8P3a0MO1qJLRkCH8KrZ3+=L66KOsMRmcbrUvYdMoKykdKoyQ@mail.gmail.com> (raw)
In-Reply-To: <20210709001328.329716-3-andrealmeid@collabora.com>

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

  parent reply	other threads:[~2021-08-17  8:51 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-09  0:13 [PATCH v5 00/11] Add futex2 syscalls André Almeida
2021-07-09  0:13 ` [PATCH v5 01/11] futex2: Implement wait and wake functions André Almeida
2021-07-09  0:13 ` [PATCH v5 02/11] futex2: Implement vectorized wait André Almeida
2021-07-14 21:19   ` Gabriel Krisman Bertazi
2021-08-17  8:50   ` Arnd Bergmann [this message]
2021-08-18 16:20     ` André Almeida
2021-07-09  0:13 ` [PATCH v5 03/11] futex2: Implement requeue operation André Almeida
2021-07-09  0:13 ` [PATCH v5 04/11] docs: locking: futex2: Add documentation André Almeida
2021-07-14 21:25   ` Gabriel Krisman Bertazi
2021-07-09  0:13 ` [PATCH v5 05/11] selftests: futex2: Add wake/wait test André Almeida
2021-07-09  0:13 ` [PATCH v5 06/11] selftests: futex2: Add timeout test André Almeida
2021-07-09  0:13 ` [PATCH v5 07/11] selftests: futex2: Add wouldblock test André Almeida
2021-07-09  0:13 ` [PATCH v5 08/11] selftests: futex2: Add waitv test André Almeida
2021-07-09  0:13 ` [PATCH v5 09/11] selftests: futex2: Add requeue test André Almeida
2021-07-09  0:13 ` [PATCH v5 10/11] perf bench: Add futex2 benchmark tests André Almeida
2021-07-09  0:13 ` [PATCH v5 11/11] kernel: Enable waitpid() for futex2 André Almeida

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAK8P3a0MO1qJLRkCH8KrZ3+=L66KOsMRmcbrUvYdMoKykdKoyQ@mail.gmail.com' \
    --to=arnd@arndb.de \
    --cc=acme@kernel.org \
    --cc=adhemerval.zanella@linaro.org \
    --cc=andrealmeid@collabora.com \
    --cc=andrey.semashev@gmail.com \
    --cc=bigeasy@linutronix.de \
    --cc=corbet@lwn.net \
    --cc=dave@stgolabs.net \
    --cc=dvhart@infradead.org \
    --cc=fweimer@redhat.com \
    --cc=joel@joelfernandes.org \
    --cc=kernel@collabora.com \
    --cc=krisman@collabora.com \
    --cc=libc-alpha@sourceware.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=malteskarupke@fastmail.fm \
    --cc=mingo@redhat.com \
    --cc=npiggin@gmail.com \
    --cc=peterz@infradead.org \
    --cc=pgriffais@valvesoftware.com \
    --cc=posk@posk.io \
    --cc=rostedt@goodmis.org \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=z.figura12@gmail.com \
    --subject='Re: [PATCH v5 02/11] futex2: Implement vectorized wait' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).