Re: [PATCH] futex: send SIGBUS if argument is not aligned on a four-byte boundary

[Peter Zijlstra <peterz@infradead.org>]

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@yandex-team.ru>
> Reported-by: Maxim Samoylov <max7255@yandex-team.ru>

Seems like a sensible idea to me.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  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))))
>