From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39378) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkaJh-0002EA-IO for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:33:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkaJc-0001Xg-O8 for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:33:25 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:49461) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkaJc-0001XW-AL for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:33:20 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 6 Jun 2013 07:33:19 -0600 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 4F0241FF0077 for ; Thu, 6 Jun 2013 07:27:14 -0600 (MDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r56DWB9d052154 for ; Thu, 6 Jun 2013 07:32:12 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r56DYbKF027308 for ; Thu, 6 Jun 2013 07:34:37 -0600 Message-ID: <51B08F5F.7030207@linux.vnet.ibm.com> Date: Thu, 06 Jun 2013 09:32:15 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1370465280-31072-1-git-send-email-coreyb@linux.vnet.ibm.com> <1370465280-31072-2-git-send-email-coreyb@linux.vnet.ibm.com> <20130606092214.GB13880@stefanha-thinkpad.redhat.com> In-Reply-To: <20130606092214.GB13880@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanb@linux.vnet.ibm.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, jschopp@linux.vnet.ibm.com, stefanha@redhat.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