lkml.org 
[lkml]   [2021]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v5 02/11] futex2: Implement vectorized wait
    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

    \
     
     \ /
      Last update: 2021-08-17 10:52    [W:2.275 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site