Hi Davidlohr,

On 09/13/2016 10:33 AM, Davidlohr Bueso wrote:



@@ -1751,12 +1820,17 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
         if (sop->sem_num >= max)
             max = sop->sem_num;
         if (sop->sem_flg & SEM_UNDO)
-            undos = 1;
+            undos = true;
         if (sop->sem_op != 0)
-            alter = 1;
+            alter = true;
+        if (sop->sem_num < SEMOPM_FAST && !dupsop) {
+            if (dup & (1 << sop->sem_num))
+                dupsop = 1;
+            else
+                dup |= 1 << sop->sem_num;
+        }
     }
At least for nsops=2, sops[0].sem_num !=sops[1].sem_num can detect absense of duplicated ops regardless of the array size.
Should we support that?

There are various individual cases like that (ie obviously nsops == 1, alter == 0, etc) where the dup detection would be unnecessary, but it seems like a stretch to go at it like this. The above will work on the common case (assuming lower sem_num of course). So I'm not particularly worried about being too smart at the dup detection.

What about the attached dup detection?

--
    Manfred
>From 140340a358dbf66b3bc6f848ca9b860e3e957e84 Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manf...@colorfullife.com>
Date: Mon, 19 Sep 2016 06:25:20 +0200
Subject: [PATCH] ipc/sem: Update duplicate sop detection

The duplicated sop detection can be improved:
- use uint64_t instead of unsigned long for the bit array
  storage, otherwise we break 32-bit archs
- support large arrays, just interpret the bit array
  as a hash array (i.e.: an operation that accesses semaphore
  0 and 64 would trigger the dupsop code, but that is
  far better than not trying at all for semnum >=64)
- support test-for-zero-and-increase, this case can use the
  fast codepath.

Untested! S-O-B only for the code, needs testing.

Signed-off-by: Manfred Spraul <manf...@colorfullife.com>
---
 ipc/sem.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index d9c743a..eda9e46 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1784,7 +1784,8 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 	int max, locknum;
 	bool undos = false, alter = false, dupsop = false;
 	struct sem_queue queue;
-	unsigned long dup = 0, jiffies_left = 0;
+	unsigned long jiffies_left = 0;
+	uint64_t dup;
 	struct ipc_namespace *ns;
 
 	ns = current->nsproxy->ipc_ns;
@@ -1816,18 +1817,32 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 		jiffies_left = timespec_to_jiffies(&_timeout);
 	}
 	max = 0;
+
+	dup = 0;
 	for (sop = sops; sop < sops + nsops; sop++) {
+		uint64_t mask;
+
 		if (sop->sem_num >= max)
 			max = sop->sem_num;
 		if (sop->sem_flg & SEM_UNDO)
 			undos = true;
-		if (sop->sem_op != 0)
+
+		/* 64: BITS_PER_UNIT64 */
+		mask = 1<<((sop->sem_num)%64);
+
+		if (dup & mask) {
+			/*
+			 * There was a previous alter access that appears
+			 * to have accessed the same semaphore, thus
+			 * use the dupsop logic.
+			 * "appears", because the detection can only check
+			 * % BITS_PER_UNIT64.
+			 */
+			dupsop = 1;
+		}
+		if (sop->sem_op != 0) {
 			alter = true;
-		if (sop->sem_num < SEMOPM_FAST && !dupsop) {
-			if (dup & (1 << sop->sem_num))
-				dupsop = 1;
-			else
-				dup |= 1 << sop->sem_num;
+			dup |= mask;
 		}
 	}
 
-- 
2.7.4

Reply via email to