From: Mingming Cao On Thu, 2003-04-24 at 17:44, Andrew Morton wrote: > Mingming Cao wrote: > > > > Hi, > > > > I have seen similar bug report from IBM. The bug report also said there > > is an oops but no kernel panic. > > Thanks. This will take some time to test. Has the patch been shown to fix > a problem in your testing? > The testing team run DOTS against this patched 2.5.66 kernel for 96hours No kernel BUG was logged in the logs (dmes, mesages). As the Bug 2050 reported, the oops happened within 24hours DOTs run. But that patch is incomplete and is not the right solution. Here I have a better solution, which I think fix the problem in the right way. 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! ipc/msg.c | 19 ++++++++++++------- ipc/sem.c | 17 ++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff -puN ipc/msg.c~semop-race-fix-2 ipc/msg.c --- 25/ipc/msg.c~semop-race-fix-2 2003-05-03 01:50:46.000000000 -0700 +++ 25-akpm/ipc/msg.c 2003-05-03 01:50:46.000000000 -0700 @@ -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 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, i break; } case IPC_RMID: - freeque (msqid); + freeque (msq, msqid); break; } err = 0; diff -puN ipc/sem.c~semop-race-fix-2 ipc/sem.c --- 25/ipc/sem.c~semop-race-fix-2 2003-05-03 01:50:46.000000000 -0700 +++ 25-akpm/ipc/sem.c 2003-05-03 01:50:46.000000000 -0700 @@ -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_arr 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 se switch(cmd){ case IPC_RMID: - freeary(semid); + freeary(sma, semid); err = 0; break; case IPC_SET: _