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

This is a discussion on [PATCH 1/1] SYSVIPC - Fix the ipc structures initialization - Kernel ; 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, ...

+ Reply to Thread
Results 1 to 2 of 2

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

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


    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

    ---
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/1] SYSVIPC - Fix the ipc structures initialization

    On Thu, 2008-10-23 at 15:57 +0200, Nadia.Derbey@bull.net wrote:
    > plain text document attachment
    > (ipc-fix-sysvipc_structures_initialization.patch)
    > 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.


    I just did my own review :-) and found out there's a possible
    factorization: the 'deleted' flag can be set once in util.c, instead of
    setting it separately for msg queues, semaphores and shm.

    Resending the patch right now. Sorry for the noise.

    Regards,
    Nadia

    >
    > Regards,
    > Nadia
    >
    >
    > Signed-off-by: Nadia Derbey
    >
    > ---
    > 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) {
    >

    --
    Nadia Derbey

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

+ Reply to Thread