Re: [RFC][PATCH 5/7] x86: convert arch_futex_atomic_op_inuser() to user_access_begin/user_access_end()

Al Viro
2020-03-24T02:08:57Z

On Mon, Mar 23, 2020 at 12:06:39PM -0700, Linus Torvalds wrote:
> On Mon, Mar 23, 2020 at 11:53 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > From: Al Viro <viro@zeniv.linux.org.uk>
> >
> > Lift stac/clac pairs from __futex_atomic_op{1,2} into arch_futex_atomic_op_inuser(),
> > fold them with access_ok() in there.
>
> So this is a deep internal macro and already has the double
> underscore, but I'm inclined to say "add the unsafe here too" for
> those __futex_atomic_opX() macros that are now called inside that
> user_access_begin/end region.
>
> And wouldn't it be lovely to get rid of the error return thing, and
> pass in a label instead, the way "usafe_get/put_user()" works too?
> That might be a separate patch from the "reorg" thing, though.

Umm...
#define __futex_atomic_op1(insn, ret, oldval, uaddr, oparg) \
asm volatile("1:\t" insn "\n" \
"2:\n" \
"\t.section .fixup,\"ax\"\n" \
"3:\tmov\t%3, %1\n" \
"\tjmp\t2b\n" \
"\t.previous\n" \
_ASM_EXTABLE_UA(1b, 3b) \
: "=r" (oldval), "=r" (ret), "+m" (*uaddr) \
: "i" (-EFAULT), "0" (oparg), "1" (0))

OK, ret wouldn't be in the list of outputs that way and
*uaddr could become an input (we only care about the address,
same as for put_user), but oldval is a genuine output -
insn is xchgl or lock xaddl, and we obviously care about the
old value fetched by it. The same goes for
#define __futex_atomic_op2(insn, ret, oldval, uaddr, oparg) \
asm volatile("1:\tmovl %2, %0\n" \
"\tmovl\t%0, %3\n" \
"\t" insn "\n" \
"2:\t" LOCK_PREFIX "cmpxchgl %3, %2\n" \
"\tjnz\t1b\n" \
"3:\n" \
"\t.section .fixup,\"ax\"\n" \
"4:\tmov\t%5, %1\n" \
"\tjmp\t3b\n" \
"\t.previous\n" \
_ASM_EXTABLE_UA(1b, 4b) \
_ASM_EXTABLE_UA(2b, 4b) \
: "=&a" (oldval), "=&r" (ret), \
"+m" (*uaddr), "=&r" (tem) \
: "r" (oparg), "i" (-EFAULT), "1" (0))
- oldval is calculated by that thing and arch_futex_atomic_op_inuser()
ends up storing it in *oval. And moving that assignment into
the inline asm will simply put *oval into the output list,
won't it?

How would you work around that?