linux-kernel - Re: [PATCH 2/5] ipc/sem: rework task wakeups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b406bcf-75e6-6f59-1de8-fe1c2c2f1315@colorfullife.com>
Date:   Tue, 13 Sep 2016 20:04:14 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Davidlohr Bueso <dave@...olabs.net>, akpm@...ux-foundation.org
Cc:     linux-kernel@...r.kernel.org, Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH 2/5] ipc/sem: rework task wakeups

Hi Davidlohr,

On 09/12/2016 01:53 PM, Davidlohr Bueso wrote:
> Hmean    sembench-sem-482    965735.00 (  0.00%)  1040313.00 (  7.72%)

[...]

> Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
> ---
>   ipc/sem.c | 268 +++++++++++++++++++-------------------------------------------
>   1 file changed, 83 insertions(+), 185 deletions(-)
Nice speedup, nice amount of removed duplicated code.
> diff --git a/ipc/sem.c b/ipc/sem.c
> index a4e8bb2fae38..86467b5b78ad 100644
[...]
>   
> -	error = get_queue_result(&queue);
> -
> -	if (error != -EINTR) {
> -		/* fast path: update_queue already obtained all requested
> -		 * resources.
> -		 * Perform a smp_mb(): User space could assume that semop()
> -		 * is a memory barrier: Without the mb(), the cpu could
> -		 * speculatively read in user space stale data that was
> -		 * overwritten by the previous owner of the semaphore.
> +	/*
> +	 * fastpath: the semop has completed, either successfully or not, from
> +	 * the syscall pov, is quite irrelevant to us at this point; we're done.
> +	 *
> +	 * We _do_ care, nonetheless, about being awoken by a signal or spuriously.
> +	 * The queue.status is checked again in the slowpath (aka after taking
> +	 * sem_lock), such that we can detect scenarios where we were awakened
> +	 * externally, during the window between wake_q_add() and wake_up_q().
> +	 */
> +	if ((error = queue.status) != -EINTR && !signal_pending(current)) {
> +		/*
> +		 * User space could assume that semop() is a memory barrier:
> +		 * Without the mb(), the cpu could speculatively read in user
> +		 * space stale data that was overwritten by the previous owner
> +		 * of the semaphore.
>   		 */
>   		smp_mb();
> -
>   		goto out_free;
>   	}
>   
>   	rcu_read_lock();
>   	sma = sem_obtain_lock(ns, semid, sops, nsops, &locknum);
What is the purpose of the !signal_pending()?
Even if there is a signal: If someone has set queue.status, then our 
semaphore operations completed and we must return that result code.
Obviously: At syscall return, the syscall return code will notice the 
pending signal and immediately the signal handler is called, but I don't 
see that this prevents us from using the fast path.

And, at least my opinion: I would avoid placing the error= into the if().

--
     Manfred

--