From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54302)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UkWOh-0004xo-O1
	for qemu-devel@nongnu.org; Thu, 06 Jun 2013 05:22:20 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UkWOg-0004LT-Hz
	for qemu-devel@nongnu.org; Thu, 06 Jun 2013 05:22:19 -0400
Received: from mail-wg0-x234.google.com ([2a00:1450:400c:c00::234]:34556)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1UkWOg-0004K9-CV
	for qemu-devel@nongnu.org; Thu, 06 Jun 2013 05:22:18 -0400
Received: by mail-wg0-f52.google.com with SMTP id z12so1244344wgg.7
	for <qemu-devel@nongnu.org>; Thu, 06 Jun 2013 02:22:17 -0700 (PDT)
Date: Thu, 6 Jun 2013 11:22:14 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130606092214.GB13880@stefanha-thinkpad.redhat.com>
References: <1370465280-31072-1-git-send-email-coreyb@linux.vnet.ibm.com>
	<1370465280-31072-2-git-send-email-coreyb@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1370465280-31072-2-git-send-email-coreyb@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Corey Bryant <coreyb@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanb@linux.vnet.ibm.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, jschopp@linux.vnet.ibm.com, stefanha@redhat.com

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.