linux-kernel - [PATCH -next 1/2] ipc/sem: simplify wait-wake loop

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 for Android: free password hash cracker in your pocket

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

Date:   Wed,  9 Nov 2016 08:26:13 -0800
From:   Davidlohr Bueso <dave@...olabs.net>
To:     akpm@...ux-foundation.org, manfred@...orfullife.com
Cc:     linux-kernel@...r.kernel.org, dave@...olabs.net,
        Davidlohr Bueso <dbueso@...e.de>
Subject: [PATCH -next 1/2] ipc/sem: simplify wait-wake loop

Instead of using the reverse goto, we can simplify the flow and
make it more language natural by just doing do-while instead.
One would hope this is the standard way (or obviously just with
a while bucle) that we do wait/wakeup handling in the kernel.
The exact same logic is kept, just more indented.

Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
---
 ipc/sem.c | 107 ++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index ebd18a7104fd..a5eaf517c8b4 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1980,71 +1980,66 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 		sma->complex_count++;
 	}
 
-sleep_again:
-	queue.status = -EINTR;
-	queue.sleeper = current;
+	do {
+		queue.status = -EINTR;
+		queue.sleeper = current;
 
-	__set_current_state(TASK_INTERRUPTIBLE);
-	sem_unlock(sma, locknum);
-	rcu_read_unlock();
+		__set_current_state(TASK_INTERRUPTIBLE);
+		sem_unlock(sma, locknum);
+		rcu_read_unlock();
 
-	if (timeout)
-		jiffies_left = schedule_timeout(jiffies_left);
-	else
-		schedule();
+		if (timeout)
+			jiffies_left = schedule_timeout(jiffies_left);
+		else
+			schedule();
 
-	/*
-	 * 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().
-	 */
-	error = READ_ONCE(queue.status);
-	if (error != -EINTR) {
 		/*
-		 * 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().
 		 */
-		smp_mb();
-		goto out_free;
-	}
-
-	rcu_read_lock();
-	sma = sem_obtain_lock(ns, semid, sops, nsops, &locknum);
-	error = READ_ONCE(queue.status);
+		error = READ_ONCE(queue.status);
+		if (error != -EINTR) {
+			/*
+			 * 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;
+		}
 
-	/*
-	 * Array removed? If yes, leave without sem_unlock().
-	 */
-	if (IS_ERR(sma)) {
-		rcu_read_unlock();
-		goto out_free;
-	}
+		rcu_read_lock();
+		sma = sem_obtain_lock(ns, semid, sops, nsops, &locknum);
+		error = READ_ONCE(queue.status);
 
-	/*
-	 * If queue.status != -EINTR we are woken up by another process.
-	 * Leave without unlink_queue(), but with sem_unlock().
-	 */
-	if (error != -EINTR)
-		goto out_unlock_free;
+		/*
+		 * Array removed? If yes, leave without sem_unlock().
+		 */
+		if (IS_ERR(sma)) {
+			rcu_read_unlock();
+			goto out_free;
+		}
 
-	/*
-	 * If an interrupt occurred we have to clean up the queue.
-	 */
-	if (timeout && jiffies_left == 0)
-		error = -EAGAIN;
+		/*
+		 * If queue.status != -EINTR we are woken up by another process.
+		 * Leave without unlink_queue(), but with sem_unlock().
+		 */
+		if (error != -EINTR)
+			goto out_unlock_free;
 
-	/*
-	 * If the wakeup was spurious, just retry.
-	 */
-	if (error == -EINTR && !signal_pending(current))
-		goto sleep_again;
+		/*
+		 * If an interrupt occurred we have to clean up the queue.
+		 */
+		if (timeout && jiffies_left == 0)
+			error = -EAGAIN;
+	} while (error == -EINTR && !signal_pending(current)); /* spurious */
 
 	unlink_queue(sma, &queue);
 
-- 
2.6.6