[PATCH] semop race fix

From: Mingming Cao <cmm@us.ibm.com>

Basically, freeary() is called with the spinlock for that semaphore set
hold.  But after the semaphore set is removed from the ID array by
calling sem_rmid(), there is no lock to protect the waiting queue for
that semaphore set.  So, if a waiter is woken up by a signal (not by the
wakeup from freeary()), it will check the q->status and q->prev fields.
At that moment, freeary() may not have a chance to update those fields
yet.

static void freeary (int id)
{
	.......
        sma = sem_rmid(id);

	......
        /* Wake up all pending processes and let them fail with EIDRM.*/
        for (q = sma->sem_pending; q; q = q->next) {
                q->status = -EIDRM;
                q->prev = NULL;
                wake_up_process(q->sleeper); /* doesn't sleep */
        }
        sem_unlock(sma);
	......
}

So I propose move sem_rmid() after the loop of waking up every waiters.
That could gurantee that when the waiters are woke up, the updates for
q->status and q->prev have already done.  Similar thing in message queue
case.  The patch is attached below. Comments are very welcomed.

I have tested this patch on 2.5.68 kernel with LTP tests, seems fine to
me. Paul, could you test this on DOTS test again? Thanks!

2 files changed, 22 insertions, 14 deletions

diff --git a/ipc/msg.c b/ipc/msg.c
index 015ff8da13ae..c1e6bb646990 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -74,7 +74,7 @@ static struct ipc_ids msg_ids;
 #define msg_buildid(id, seq) \
 	ipc_buildid(&msg_ids, id, seq)
 
-static void freeque (int id);
+static void freeque (struct msg_queue *msq, int id);
 static int newque (key_t key, int msgflg);
 #ifdef CONFIG_PROC_FS
 static int sysvipc_msg_read_proc(char *buffer, char **start, off_t offset, int length, int *eof, void *data);
@@ -272,16 +272,21 @@ static void expunge_all(struct msg_queue* msq, int res)
 		wake_up_process(msr->r_tsk);
 	}
 }
-
-static void freeque (int id)
+/* 
+ * freeque() wakes up waiters on the sender and receiver waiting queue, 
+ * removes the message queue from message queue ID 
+ * array, and cleans up all the messages associated with this queue.
+ *
+ * msg_ids.sem and the spinlock for this message queue is hold
+ * before freeque() is called. msg_ids.sem remains locked on exit.
+ */
+static void freeque (struct msg_queue *msq, int id)
 {
-	struct msg_queue *msq;
 	struct list_head *tmp;
 
-	msq = msg_rmid(id);
-
 	expunge_all(msq,-EIDRM);
 	ss_wakeup(&msq->q_senders,1);
+	msq = msg_rmid(id);
 	msg_unlock(msq);
 		
 	tmp = msq->q_messages.next;
@@ -574,7 +579,7 @@ asmlinkage long sys_msgctl (int msqid, int cmd, struct msqid_ds *buf)
 		break;
 	}
 	case IPC_RMID:
-		freeque (msqid); 
+		freeque (msq, msqid); 
 		break;
 	}
 	err = 0;
diff --git a/ipc/sem.c b/ipc/sem.c
index d965818ea0ee..411e07803760 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -79,7 +79,7 @@
 static struct ipc_ids sem_ids;
 
 static int newary (key_t, int, int);
-static void freeary (int id);
+static void freeary (struct sem_array *sma, int id);
 #ifdef CONFIG_PROC_FS
 static int sysvipc_sem_read_proc(char *buffer, char **start, off_t offset, int length, int *eof, void *data);
 #endif
@@ -405,16 +405,16 @@ static int count_semzcnt (struct sem_array * sma, ushort semnum)
 	return semzcnt;
 }
 
-/* Free a semaphore set. */
-static void freeary (int id)
+/* Free a semaphore set. freeary() is called with sem_ids.sem down and
+ * the spinlock for this semaphore set hold. sem_ids.sem remains locked
+ * on exit.
+ */
+static void freeary (struct sem_array *sma, int id)
 {
-	struct sem_array *sma;
 	struct sem_undo *un;
 	struct sem_queue *q;
 	int size;
 
-	sma = sem_rmid(id);
-
 	/* Invalidate the existing undo structures for this semaphore set.
 	 * (They will be freed without any further action in sem_exit()
 	 * or during the next semop.)
@@ -428,6 +428,9 @@ static void freeary (int id)
 		q->prev = NULL;
 		wake_up_process(q->sleeper); /* doesn't sleep */
 	}
+
+	/* Remove the semaphore set from the ID array*/
+	sma = sem_rmid(id);
 	sem_unlock(sma);
 
 	used_sems -= sma->sem_nsems;
@@ -764,7 +767,7 @@ static int semctl_down(int semid, int semnum, int cmd, int version, union semun
 
 	switch(cmd){
 	case IPC_RMID:
-		freeary(semid);
+		freeary(sma, semid);
 		err = 0;
 		break;
 	case IPC_SET: