ipc: move rcu_read_unlock() out of sem_unlock() and into callers
The IPC locking is a mess, and sem_unlock() unlocks not only the
semaphore spinlock, it also drops the rcu read lock. Unlike sem_lock(),
which just gets the spin-lock, and expects the caller to get the rcu
read lock.
This all makes things very hard to follow, and it's very confusing when
you take the rcu read lock in one function, and then release it in
another. And it has caused actual bugs: the sem_obtain_lock() function
ended up dropping the RCU read lock twice in one error path, because it
first did the sem_unlock(), and then did a rcu_read_unlock() to match
the rcu_read_lock() it had done.
This is just a totally mindless "remove rcu_read_unlock() from
sem_unlock() and add it immediately after each caller" (except for the
aforementioned bug where we did too many rcu_read_unlock(), and in
find_alloc_undo() where we just got the rcu_read_lock() to correct for
the fact that sem_unlock would immediately drop it again).
We can (and should) clean things up further, but this fixes the bug with
the minimal amount of subtlety.
Reviewed-by: Davidlohr Bueso <davidlohr.bueso@hp.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
diff --git a/ipc/sem.c b/ipc/sem.c
index 4734e9c..4b4139f 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -264,7 +264,6 @@
struct sem *sem = sma->sem_base + locknum;
spin_unlock(&sem->lock);
}
- rcu_read_unlock();
}
/*
@@ -332,6 +331,7 @@
{
sem_lock_and_putref(sma);
sem_unlock(sma, -1);
+ rcu_read_unlock();
}
static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s)
@@ -435,6 +435,7 @@
sma->sem_nsems = nsems;
sma->sem_ctime = get_seconds();
sem_unlock(sma, -1);
+ rcu_read_unlock();
return sma->sem_perm.id;
}
@@ -874,6 +875,7 @@
/* Remove the semaphore set from the IDR */
sem_rmid(ns, sma);
sem_unlock(sma, -1);
+ rcu_read_unlock();
wake_up_sem_queue_do(&tasks);
ns->used_sems -= sma->sem_nsems;
@@ -1055,6 +1057,7 @@
/* maybe some queued-up processes were waiting for this */
do_smart_update(sma, NULL, 0, 0, &tasks);
sem_unlock(sma, -1);
+ rcu_read_unlock();
wake_up_sem_queue_do(&tasks);
return 0;
}
@@ -1104,10 +1107,12 @@
if(nsems > SEMMSL_FAST) {
if (!ipc_rcu_getref(sma)) {
sem_unlock(sma, -1);
+ rcu_read_unlock();
err = -EIDRM;
goto out_free;
}
sem_unlock(sma, -1);
+ rcu_read_unlock();
sem_io = ipc_alloc(sizeof(ushort)*nsems);
if(sem_io == NULL) {
sem_putref(sma);
@@ -1117,6 +1122,7 @@
sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma, -1);
+ rcu_read_unlock();
err = -EIDRM;
goto out_free;
}
@@ -1124,6 +1130,7 @@
for (i = 0; i < sma->sem_nsems; i++)
sem_io[i] = sma->sem_base[i].semval;
sem_unlock(sma, -1);
+ rcu_read_unlock();
err = 0;
if(copy_to_user(array, sem_io, nsems*sizeof(ushort)))
err = -EFAULT;
@@ -1164,6 +1171,7 @@
sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma, -1);
+ rcu_read_unlock();
err = -EIDRM;
goto out_free;
}
@@ -1210,6 +1218,7 @@
out_unlock:
sem_unlock(sma, -1);
+ rcu_read_unlock();
out_wakeup:
wake_up_sem_queue_do(&tasks);
out_free:
@@ -1295,6 +1304,7 @@
out_unlock:
sem_unlock(sma, -1);
+ rcu_read_unlock();
out_up:
up_write(&sem_ids(ns).rw_mutex);
return err;
@@ -1443,9 +1453,11 @@
}
/* step 3: Acquire the lock on semaphore array */
+ /* This also does the rcu_read_lock() */
sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma, -1);
+ rcu_read_unlock();
kfree(new);
un = ERR_PTR(-EIDRM);
goto out;
@@ -1472,7 +1484,6 @@
success:
spin_unlock(&ulp->lock);
- rcu_read_lock();
sem_unlock(sma, -1);
out:
return un;
@@ -1648,6 +1659,7 @@
sleep_again:
current->state = TASK_INTERRUPTIBLE;
sem_unlock(sma, locknum);
+ rcu_read_unlock();
if (timeout)
jiffies_left = schedule_timeout(jiffies_left);
@@ -1709,6 +1721,7 @@
out_unlock_free:
sem_unlock(sma, locknum);
+ rcu_read_unlock();
out_wakeup:
wake_up_sem_queue_do(&tasks);
out_free:
@@ -1801,6 +1814,7 @@
* exactly the same semid. Nothing to do.
*/
sem_unlock(sma, -1);
+ rcu_read_unlock();
continue;
}
@@ -1841,6 +1855,7 @@
INIT_LIST_HEAD(&tasks);
do_smart_update(sma, NULL, 0, 1, &tasks);
sem_unlock(sma, -1);
+ rcu_read_unlock();
wake_up_sem_queue_do(&tasks);
kfree_rcu(un, rcu);