> On Mon, 2008-10-27 at 12:04 +0100, Nadia Derbey wrote:
>> On Mon, 2008-10-27 at 11:32 +0100,
cboulte@gmail.com wrote:
>> > On Mon, Oct 27, 2008 at 8:28 AM, <Nadia.Derbey@bull.net> wrote:
>> > >
>> > > 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().
>> > >
>> > >
>> > > Regards,
>> > > Nadia
>> > >
>> > >
>> > > Signed-off-by: Nadia Derbey <Nadia.Derbey@bull.net>
>> > >
>> > > ---
>> > > ipc/util.c | 16 ++++++++++++++--
>> > > 1 file changed, 14 insertions(+), 2 deletions(-)
>> > >
>> > > 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-24 17:48:33.000000000 +0200
>> > > @@ -266,6 +266,17 @@ int ipc_addid(struct ipc_ids* ids, struc
>> > > if (ids->in_use >= size)
>> > > return -ENOSPC;
>> > >
>> > > + spin_lock_init(&new->lock);
>> > > +
>> > > + /*
>> > > + * We have a window between the time new is inserted into the idr
>> > > + * tree and the time it is actually locked.
>> > > + * In order to be safe during that window set the new ipc structure
>> > > + * as deleted: a concurrent ipc_lock() will see it as not present
>> > > + * until the initialization phase is complete.
>> > > + */
>> > > + new->deleted = 1;
>> > > +
>> > > err = idr_get_new(&ids->ipcs_idr, new, &id);
>> > > if (err)
>> > > return err;
>> > > @@ -280,10 +291,11 @@ 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);
>> > > +
>> > > + new->deleted = 0;
>> > > +
>> > > return id;
>> > > }
>> > >
>> > >
>> > > --
>> > >
>> >
>> > Still got the lock... I'm using a 4 cpus node: Intel Xeon @ 2.8GHz...
>> > don't know if it has an impact.
>> ???
>> The bad new, is that it becomes unreprodicible on my side.
>> For my part, I've got 2 2.8 GHz Xeon CPUs.
>>
>> Will review the code once more.
>>
>> Thanks!
>> Nadia
>>
>> > The only way I found to have no lock, it's to spin_lock the ipc
>> > _before_ inserting it into the idr.
>> >
>> > Best regards, c.
>> >
>
> I agree with you that it's more logical and correct to take the lock
> before inserting the ipc structure (i.e. making it visible to readers).
>
> But I wanted to understand what's wrong with
> 1. new->lock init
> 2. new->deleted = 1
> 3. insert(new)
>
> I've been looking at the code again and again and the only thing I see
> could have happened, is that instructions have been reordered and the
> insertion done before the lock actually being initialized.
> This means that a memory barrier is missing (this would explain why your
> fix works: the spin_lock acts as a barrier).
> But this memory barrier is supposed to be invoked by
> rcu_assign_pointer() in idr_get_new(). So may be there's a problem with
> the idr code.
> Before going into a review of this code, I'd like to confirm what I'm
> saying, doing the following (I'm sorry to ask you do it, but I can't
> reproduce the problem in my side anymore): would you mind adding a
> smp_wmb() just before the idr_get_new in ipc_addid() and tell me if this
> solves the problem.
> (BTW, I didn't ask you before, but I guess you're getting the same call
> trace?)
>
> Regards,
> Nadia
>
> --
> Nadia Derbey <Nadia.Derbey@bull.net>
>
>
