Re: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly

[Paolo Bonzini]

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