From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNTjR-0007OU-UL for qemu-devel@nongnu.org; Fri, 20 May 2011 13:43:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QNTjR-0003Ql-20 for qemu-devel@nongnu.org; Fri, 20 May 2011 13:43:25 -0400 Received: from mail-ww0-f41.google.com ([74.125.82.41]:64411) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNTjQ-0003Qg-RS for qemu-devel@nongnu.org; Fri, 20 May 2011 13:43:25 -0400 Received: by wwi18 with SMTP id 18so704880wwi.4 for ; Fri, 20 May 2011 10:43:23 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4DD6A839.1020104@redhat.com> Date: Fri, 20 May 2011 19:43:21 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1305630067-2119-1-git-send-email-pbonzini@redhat.com> <1305630067-2119-14-git-send-email-pbonzini@redhat.com> <20110520160422.GM4466@lst.de> In-Reply-To: <20110520160422.GM4466@lst.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: qemu-devel@nongnu.org On 05/20/2011 06:04 PM, Christoph Hellwig wrote: >> -void scsi_req_enqueue(SCSIRequest *req) >> +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf) >> { >> + int32_t rc; >> assert(!req->enqueued); >> scsi_req_ref(req); >> req->enqueued = true; >> QTAILQ_INSERT_TAIL(&req->dev->requests, req, next); >> + >> + /* Make sure the request doesn't disappear under send_command's feet. */ >> + scsi_req_ref(req); >> + rc = req->dev->info->send_command(req, buf); >> + scsi_req_unref(req); >> + return rc; > > How would it disappear given that we grabbed another reference just before? Welcome to the wonderful world of nested callbacks. :( Suppose send_command completes a request. scsi_req_complete then dequeues it (which undoes the reference above), and calls the device who owned it. The device who owned the request then presumably NULLs a pointer and drops the last reference. The request is then freed. This was exactly the kind of scenario that I was worried about when I thought about reference counting. I couldn't actually put my fingers on it, but I knew it was broken somewhere, and indeed a use-after-free was reported less than a month later. It is not strictly necessary to wrap send_command with a ref/unref pair, but it is quite convenient when writing send_command. It also mirrors what I do around scsi_req_complete and scsi_req_cancel. > That probably needs a bit more documentation here. Also why not move > the two scsi_req_ref calls together? Because one is logically related to putting the request in the queue, while the other is related to the send_command op. Paolo