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]

Date:   Wed, 14 Sep 2016 08:45:48 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Manfred Spraul <manfred@...orfullife.com>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH 2/5] ipc/sem: rework task wakeups

On Tue, 13 Sep 2016, Manfred Spraul wrote:

>>+	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.

It was a way of detecting being awoken by an unrelated event while the task
is marked for wakeup and wake_up_process is still not called and force the
slowpath. The same window that get_queue_result deals with by busy waiting
when IN_WAKEUP.

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

Right, and we cannot race with sys_exit. Will drop the signal check.

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

Sure, agreed.

Thanks,
Davidlohr