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
