Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 May 2020 18:27:07 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc:     linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Darren Hart <dvhart@infradead.org>,
        Maxim Samoylov <max7255@yandex-team.ru>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-api@vger.kernel.org
Subject: Re: [PATCH] futex: send SIGBUS if argument is not aligned on a
 four-byte boundary
Message-ID: <20200515162707.GI2978@hirez.programming.kicks-ass.net>
References: <158955700764.647498.18025770126733698386.stgit@buzz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <158955700764.647498.18025770126733698386.stgit@buzz>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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