[PATCH 1/1] SYSVIPC - Fix the ipc structures initialization

From: Nadia . Derbey
Date: Thu Oct 23 2008 - 09:58:35 EST



This patch is a fix for Bugzilla bug
http://bugzilla.kernel.org/show_bug.cgi?id=11796.

To summarize, a simple testcase is concurrently running an infinite loop on
msgctl(IPC_STAT) and a call to msgget():

while (1)
msgctl(id, IPC_STAT) 1
|
|
|
2 id = msgget(key, IPC_CREAT)
|
|
|

In the interval [1-2], the id doesn't exist yet.

In that test, the problem is the following:
When we are calling ipc_addid() from msgget() the msq structure is not
completely initialized. So idr_get_new() is inserting a pointer into the
idr tree, and the structure which is pointed to has, among other fields,
its lock uninitialized.

Since msgctl(IPC_STAT) is looping while (1), idr_find() returns the
pointer as soon as it is inserted into the IDR tree. And ipc_lock()
calls spin_lock(&mqs->lock), while we have not initialized that lock
yet.

This patch moves the spin_lock_init() before the call to ipc_addid().
It also sets the "deleted" flag to 1 in the window between msg structure
allocation and msg structure locking in ipc_addid().

The fix has been applied to semaphores and shm segments too.

Regards,
Nadia


Signed-off-by: Nadia Derbey <Nadia.Derbey@xxxxxxxx>

---
ipc/msg.c | 9 +++++++++
ipc/sem.c | 9 +++++++++
ipc/shm.c | 9 +++++++++
ipc/util.c | 3 ++-
4 files changed, 29 insertions(+), 1 deletion(-)

Index: linux-2.6.27/ipc/msg.c
===================================================================
--- linux-2.6.27.orig/ipc/msg.c 2008-10-23 15:20:46.000000000 +0200
+++ linux-2.6.27/ipc/msg.c 2008-10-23 16:38:59.000000000 +0200
@@ -191,6 +191,15 @@ static int newque(struct ipc_namespace *
msq->q_perm.mode = msgflg & S_IRWXUGO;
msq->q_perm.key = key;

+ /*
+ * We have a window between the time msq is inserted into the idr
+ * tree (done in ipc_addid()) and the time it is actually locked.
+ * In order to be safe during that window set the msq as deleted:
+ * a concurrent ipc_lock() will see it as not present until the
+ * initialization phase is complete.
+ */
+ msq->q_perm.deleted = 1;
+
msq->q_perm.security = NULL;
retval = security_msg_queue_alloc(msq);
if (retval) {
Index: linux-2.6.27/ipc/util.c
===================================================================
--- linux-2.6.27.orig/ipc/util.c 2008-10-23 15:20:46.000000000 +0200
+++ linux-2.6.27/ipc/util.c 2008-10-23 15:33:37.000000000 +0200
@@ -266,6 +266,8 @@ int ipc_addid(struct ipc_ids* ids, struc
if (ids->in_use >= size)
return -ENOSPC;

+ spin_lock_init(&new->lock);
+
err = idr_get_new(&ids->ipcs_idr, new, &id);
if (err)
return err;
@@ -280,7 +282,6 @@ int ipc_addid(struct ipc_ids* ids, struc
ids->seq = 0;

new->id = ipc_buildid(id, new->seq);
- spin_lock_init(&new->lock);
new->deleted = 0;
rcu_read_lock();
spin_lock(&new->lock);
Index: linux-2.6.27/ipc/sem.c
===================================================================
--- linux-2.6.27.orig/ipc/sem.c 2008-10-23 15:20:46.000000000 +0200
+++ linux-2.6.27/ipc/sem.c 2008-10-23 15:57:27.000000000 +0200
@@ -256,6 +256,15 @@ static int newary(struct ipc_namespace *
sma->sem_perm.mode = (semflg & S_IRWXUGO);
sma->sem_perm.key = key;

+ /*
+ * We have a window between the time sma is inserted into the idr
+ * tree (done in ipc_addid()) and the time it is actually locked.
+ * In order to be safe during that window set the sma as deleted:
+ * a concurrent ipc_lock() will see it as not present until the
+ * initialization phase is complete.
+ */
+ sma->sem_perm.deleted = 1;
+
sma->sem_perm.security = NULL;
retval = security_sem_alloc(sma);
if (retval) {
Index: linux-2.6.27/ipc/shm.c
===================================================================
--- linux-2.6.27.orig/ipc/shm.c 2008-10-23 15:20:46.000000000 +0200
+++ linux-2.6.27/ipc/shm.c 2008-10-23 15:56:32.000000000 +0200
@@ -355,6 +355,15 @@ static int newseg(struct ipc_namespace *
shp->shm_perm.mode = (shmflg & S_IRWXUGO);
shp->mlock_user = NULL;

+ /*
+ * We have a window between the time shp is inserted into the idr
+ * tree (done in ipc_addid()) and the time it is actually locked.
+ * In order to be safe during that window set the shp as deleted:
+ * a concurrent ipc_lock() will see it as not present until the
+ * initialization phase is complete.
+ */
+ shp->shm_perm.deleted = 1;
+
shp->shm_perm.security = NULL;
error = security_shm_alloc(shp);
if (error) {

--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/