On Fri, May 15, 2020 at 06:36:47PM +0300, Konstantin Khlebnikov wrote:
> Userspace implementations of mutexes (including glibc) in some cases
> retries operation without checking error code from syscall futex.
> This is good for performance because most errors are impossible when
> locking code trusts itself.
>
> Some errors which could came from outer code are handled automatically,
> for example invalid address triggers SIGSEGV on atomic fast path.
>
> But one case turns into nasty busy-loop: when address is unaligned.
> futex(FUTEX_WAIT) returns EINVAL immediately and loop goes to retry.
>
> Example which loops inside second call rather than hung peacefully:
>
> #include <stdlib.h>
> #include <pthread.h>
>
> int main(int argc, char **argv)
> {
> char buf[sizeof(pthread_mutex_t) + 1];
> pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);
>
> pthread_mutex_init(mutex, NULL);
> pthread_mutex_lock(mutex);
> pthread_mutex_lock(mutex);
> }
>
> It seems there is no practical usage for calling syscall futex for
> unaligned address. This may be only bug in user space. Let's help
> and handle this gracefully without adding extra code on fast path.
>
> This patch sends SIGBUS signal to slay task and break busy-loop.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
> Reported-by: Maxim Samoylov <max7255@xxxxxxxxxxxxxx>
Seems like a sensible idea to me.
Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> kernel/futex.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index b59532862bc0..8a6d35fa56bc 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -508,10 +508,21 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
>
> /*
> * The futex address must be "naturally" aligned.
> + * Also send signal to break busy-loop if user-space ignore error.
> + * EFAULT case should trigger SIGSEGV at access from user-space.
> */
> key->both.offset = address % PAGE_SIZE;
> - if (unlikely((address % sizeof(u32)) != 0))
> + if (unlikely((address % sizeof(u32)) != 0)) {
> + struct kernel_siginfo info;
> +
> + clear_siginfo(&info);
> + info.si_signo = SIGBUS;
> + info.si_code = BUS_ADRALN;
> + info.si_addr = uaddr;
> + force_sig_info(&info);
> +
> return -EINVAL;
> + }
> address -= key->both.offset;
>
> if (unlikely(!access_ok(uaddr, sizeof(u32))))
>
