* Re: [PATCH] futex: improve user space accesses [not found] <20241122193305.7316-1-torvalds@linux-foundation.org> @ 2024-12-08 22:54 ` Andreas Schwab 2024-12-09 0:32 ` Linus Torvalds 0 siblings, 1 reply; 4+ messages in thread From: Andreas Schwab @ 2024-12-08 22:54 UTC (permalink / raw) To: Linus Torvalds; +Cc: Josh Poimboeuf, LKML, x86, linuxppc-dev 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." ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] futex: improve user space accesses 2024-12-08 22:54 ` [PATCH] futex: improve user space accesses Andreas Schwab @ 2024-12-09 0:32 ` Linus Torvalds 2024-12-09 8:00 ` Christophe Leroy 2024-12-09 18:32 ` Andreas Schwab 0 siblings, 2 replies; 4+ messages in thread From: Linus Torvalds @ 2024-12-09 0:32 UTC (permalink / raw) To: Andreas Schwab; +Cc: Josh Poimboeuf, LKML, x86, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] futex: improve user space accesses 2024-12-09 0:32 ` Linus Torvalds @ 2024-12-09 8:00 ` Christophe Leroy 2024-12-09 18:32 ` Andreas Schwab 1 sibling, 0 replies; 4+ messages in thread From: Christophe Leroy @ 2024-12-09 8:00 UTC (permalink / raw) To: Linus Torvalds, Andreas Schwab; +Cc: Josh Poimboeuf, LKML, x86, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] futex: improve user space accesses 2024-12-09 0:32 ` Linus Torvalds 2024-12-09 8:00 ` Christophe Leroy @ 2024-12-09 18:32 ` Andreas Schwab 1 sibling, 0 replies; 4+ messages in thread From: Andreas Schwab @ 2024-12-09 18:32 UTC (permalink / raw) To: Linus Torvalds; +Cc: Josh Poimboeuf, LKML, x86, linuxppc-dev 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." ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-09 18:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241122193305.7316-1-torvalds@linux-foundation.org>
2024-12-08 22:54 ` [PATCH] futex: improve user space accesses Andreas Schwab
2024-12-09 0:32 ` Linus Torvalds
2024-12-09 8:00 ` Christophe Leroy
2024-12-09 18:32 ` Andreas Schwab
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).