Re: [Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 06/06/2013 05:22 AM, Stefan Hajnoczi wrote:
> On Wed, Jun 05, 2013 at 04:47:59PM -0400, Corey Bryant wrote:
>> +/*
>> + * Coroutine that reads a blob from the drive asynchronously
>> + */
>> +static void coroutine_fn tpm_nvram_co_read(void *opaque)
>> +{
>> +    TPMNvramRWRequest *rwr = opaque;
>> +
>> +    *rwr->blob_r = g_malloc(rwr->size);
>> +
>> +    rwr->rc = bdrv_pread(rwr->bdrv,
>> +                         rwr->offset,
>> +                         *rwr->blob_r,
>> +                         rwr->size);
>> +    if (rwr->rc != rwr->size) {
>> +        g_free(*rwr->blob_r);
>> +        *rwr->blob_r = NULL;
>> +    }
>> +
>> +    qemu_mutex_lock(&rwr->completion_mutex);
>
> Race condition: we must only store rwr->rc while holding
> ->completion_mutex.  Otherwise the other thread may see ->rc and
> g_free(rwr) before we leave this function, causing us to operate on
> freed memory.
>
> I suggest storing rc into a local variable first and then assigning to
> rwr->rc here.
>
>> +/*
>> + * Enter a coroutine to write a blob to the drive
>> + */
>> +static void tpm_nvram_do_co_write(TPMNvramRWRequest *rwr)
>> +{
>> +    int rc;
>> +    Coroutine *co;
>> +
>> +    rc = tpm_nvram_adjust_size(rwr->bdrv, rwr->offset, rwr->size);
>> +    if (rc < 0) {
>> +        rwr->rc = rc;
>> +        return;
>> +    }
>
> Do this inside the tpm_nvram_co_write() coroutine so error return still
> signals the condvar.  Right now the other thread may miss completion and
> deadlock.
>
>

Thanks for the review.  Sending a v3 out to the list now.

-- 
Regards,
Corey Bryant