Re: [PATCH] futex: improve user space accesses

Re: [PATCH] futex: improve user space accesses

[Andreas Schwab]

On Nov 22 2024, Linus Torvalds wrote:

> Josh Poimboeuf reports that he got a "will-it-scale.per_process_ops 1.9%
> improvement" report for his patch that changed __get_user() to use
> pointer masking instead of the explicit speculation barrier.  However,
> that patch doesn't actually work in the general case, because some (very
> bad) architecture-specific code actually depends on __get_user() also
> working on kernel addresses.
>
> A profile showed that the offending __get_user() was the futex code,
> which really should be fixed up to not use that horrid legacy case.
> Rewrite futex_get_value_locked() to use the modern user acccess helpers,
> and inline it so that the compiler not only avoids the function call for
> a few instructions, but can do CSE on the address masking.

This breaks userspace on ppc32.  As soon as /init in the initrd is
started the kernel hangs (without any messages).

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] futex: improve user space accesses

[Linus Torvalds]

On Sun, 8 Dec 2024 at 14:54, Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> This breaks userspace on ppc32.  As soon as /init in the initrd is
> started the kernel hangs (without any messages).

Funky, funky. Most of the diff is the code movement (and some small
x86-specific stuff), so for ppc, the only part that should be relevant
is the futex_get_value_locked().

And since ppc doesn't do the masked user access thing, so it
*literally* boils down to just that

        if (!user_read_access_begin(from, sizeof(*from)))
                return -EFAULT;
        unsafe_get_user(val, from, Efault);
        user_access_end();

path.

Ahh... And now that I write that out, the bug is obvious: it should be using

        user_read_access_end();

to match up with the user_read_access_begin().

And yeah, ppc is the only platform that has that
"read-vs-write-vs-both" thing, so this bug is not visible anywhere
else.

IOW, does this one-liner fix it for you?

  --- a/kernel/futex/futex.h
  +++ b/kernel/futex/futex.h
  @@ -265,7 +265,7 @@
        else if (!user_read_access_begin(from, sizeof(*from)))
                return -EFAULT;
        unsafe_get_user(val, from, Efault);
  -     user_access_end();
  +     user_read_access_end();
        *dest = val;
        return 0;
   Efault:

I bet it does, but I'll wait for confirmation before actually
committing that fix.

Thanks,

                Linus

Re: [PATCH] futex: improve user space accesses

[Christophe Leroy]

Le 09/12/2024 à 01:32, Linus Torvalds a écrit :
> On Sun, 8 Dec 2024 at 14:54, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> This breaks userspace on ppc32.  As soon as /init in the initrd is
>> started the kernel hangs (without any messages).
> 
> Funky, funky. Most of the diff is the code movement (and some small
> x86-specific stuff), so for ppc, the only part that should be relevant
> is the futex_get_value_locked().
> 
> And since ppc doesn't do the masked user access thing, so it
> *literally* boils down to just that
> 
>          if (!user_read_access_begin(from, sizeof(*from)))
>                  return -EFAULT;
>          unsafe_get_user(val, from, Efault);
>          user_access_end();
> 
> path.
> 
> Ahh... And now that I write that out, the bug is obvious: it should be using
> 
>          user_read_access_end();
> 
> to match up with the user_read_access_begin().

Yes indeed, especially on book3s/32, which is only able to write-protect 
user accesses. On that platform user_read_access_...() are no-ops.

user_access_end() and user_write_access_end() are similar, and rely on a 
thread var stored by user_access_begin(). When calling that 
user_access_end() without prior call to user_access_begin(), that var 
has value ~0 instead of the address of the user segment being accessed, 
and ~0 is a kernel address so user_access_end() applies some user 
segment flags to a kernel segment which most likely leads to a complete 
mess allthough I'm not able to trigger the hang with QEMU.

> 
> And yeah, ppc is the only platform that has that
> "read-vs-write-vs-both" thing, so this bug is not visible anywhere
> else.
> 
> IOW, does this one-liner fix it for you?
> 
>    --- a/kernel/futex/futex.h
>    +++ b/kernel/futex/futex.h
>    @@ -265,7 +265,7 @@
>          else if (!user_read_access_begin(from, sizeof(*from)))
>                  return -EFAULT;
>          unsafe_get_user(val, from, Efault);
>    -     user_access_end();
>    +     user_read_access_end();
>          *dest = val;
>          return 0;
>     Efault:
> 
> I bet it does, but I'll wait for confirmation before actually
> committing that fix.
> 

You'll need the same change in the Efault leg.

Christophe

Re: [PATCH] futex: improve user space accesses

[Andreas Schwab]

On Dez 08 2024, Linus Torvalds wrote:

> IOW, does this one-liner fix it for you?
>
>   --- a/kernel/futex/futex.h
>   +++ b/kernel/futex/futex.h
>   @@ -265,7 +265,7 @@
>         else if (!user_read_access_begin(from, sizeof(*from)))
>                 return -EFAULT;
>         unsafe_get_user(val, from, Efault);
>   -     user_access_end();
>   +     user_read_access_end();
>         *dest = val;
>         return 0;
>    Efault:

Thanks, I can confirm that this fixed the crash (changing both arms as
pointed out by Christophe).

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."