Re: [Qemu-devel] [PATCH] block: Fix race in gluster_finish_aiocb

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/08/2013 17:24, Stefan Hajnoczi ha scritto:
> On Wed, Aug 21, 2013 at 10:02:47AM +0800, Asias He wrote:
>> In block/gluster.c, we have
>>
>> gluster_finish_aiocb
>> {
>>    if (retval != sizeof(acb)) {
>>       qemu_mutex_lock_iothread(); /* We are in gluster thread context */
>>       ...
>>       qemu_mutex_unlock_iothread();
>>    }
>> }
>>
>> qemu tools, e.g. qemu-img, might race here because
>> qemu_mutex_{lock,unlock}_iothread are a nop operation and
>> gluster_finish_aiocb is in the gluster thread context.
>>
>> To fix, we introduce our own mutex for qemu tools.
> 
> I think we need to look more closely at the error code path:
> 
> acb->ret = ret;
> retval = qemu_write_full(s->fds[GLUSTER_FD_WRITE], &acb, sizeof(acb));
> if (retval != sizeof(acb)) {
>     /*
>      * Gluster AIO callback thread failed to notify the waiting
>      * QEMU thread about IO completion.
>      *
>      * Complete this IO request and make the disk inaccessible for
>      * subsequent reads and writes.
>      */
>     error_report("Gluster failed to notify QEMU about IO completion");
> 
>     qemu_mutex_lock_iothread(); /* We are in gluster thread context */
>     acb->common.cb(acb->common.opaque, -EIO);
>     qemu_aio_release(acb);
>     close(s->fds[GLUSTER_FD_READ]);
>     close(s->fds[GLUSTER_FD_WRITE]);
> 
> Is it safe to close the fds?  There is a race here:
> 
> 1. Another thread opens a new file descriptor and gets GLUSTER_FD_READ or
>    GLUSTER_FD_WRITE's old fd value.
> 2. Another gluster thread invokes the callback and does
>    qemu_write_full(s->fds[GLUSTER_FD_WRITE], ...).
> 
> Since the mutex doesn't protect s->fds[] this is possible.
> 
> Maybe a simpler solution for request completion is:
> 
> 1. Linked list of completed acbs.
> 2. Mutex to protect the linked list.
> 3. EventNotifier to signal iothread.

We could just use a bottom half, too.  Add a bottom half to acb,
schedule it in gluster_finish_aiocb, delete it in the bottom half's own
callback.

Paolo

> Then this function becomes:
> 
> static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
> {
>     GlusterAIOCB *acb = arg;
>     BlockDriverState *bs = acb->common.bs;
>     BDRVGlusterState *s = bs->opaque;
>     int retval;
> 
>     acb->ret = ret;
> 
>     qemu_mutex_lock(&s->finish_list_lock);
>     QSIMPLEQ_INSERT_TAIL(&s->finish_list, acb, list);
>     qemu_mutex_unlock(&s->finish_list_lock);
> 
>     event_notifier_set(&s->finish_list_notifier);
> }
> 
> Stefan
> 
>