block: Fix race in gluster_finish_aiocb

On Wed, Aug 21, 2013 at 05:40:11PM +0200, Paolo Bonzini wrote:
> 
> 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.

I tried this approach (the patch at the end of this mail), but see this
occasional errors, doesn't happen always. Any clues on what am I missing here ?

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffcbfff700 (LWP 11301)]
0x000055555597156b in event_notifier_set (e=0xa4)
    at util/event_notifier-posix.c:97
97	        ret = write(e->wfd, &value, sizeof(value));
(gdb) bt
#0  0x000055555597156b in event_notifier_set (e=0xa4)
    at util/event_notifier-posix.c:97
#1  0x00005555555dd3cd in aio_notify (ctx=0x0) at async.c:233
#2  0x00005555555dd00f in qemu_bh_schedule (bh=0x7fff300008e0) at async.c:128
#3  0x00005555556002da in gluster_finish_aiocb (fd=0x5555562ae5d0, ret=4096, 
    arg=0x7fffd00419c0) at block/gluster.c:409
#4  0x00007ffff5e70cdc in glfs_io_async_cbk (ret=4096, frame=0x0, data=
    0x555556443ee0) at glfs-fops.c:567
#5  0x00007ffff5c2843e in synctask_wrap (old_task=0x555556365940)
    at syncop.c:133
#6  0x00007ffff3b03370 in ?? () from /lib64/libc.so.6
#7  0x0000000000000000 in ?? ()
(gdb) up
#1  0x00005555555dd3cd in aio_notify (ctx=0x0) at async.c:233
233	    event_notifier_set(&ctx->notifier);
(gdb) up
#2  0x00005555555dd00f in qemu_bh_schedule (bh=0x7fff300008e0) at async.c:128
128	    aio_notify(bh->ctx);
(gdb) p *bh
$1 = {ctx = 0x0, cb = 0x5555555ffdcd <qemu_gluster_aio_bh>, opaque = 
    0x7fffd00419c0, next = 0x555556345e70, scheduled = false, idle = false, 
  deleted = true}


gluster: Use BH mechanism for AIO completion

Replace the existing pipe based AIO completion handing by BH based method.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 block/gluster.c |   69 ++++++-------------------------------------------------
 1 file changed, 8 insertions(+), 61 deletions(-)

Il 23/08/2013 08:48, Bharata B Rao ha scritto:
> On Wed, Aug 21, 2013 at 05:40:11PM +0200, Paolo Bonzini wrote:
>>
>> 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.
> 
> I tried this approach (the patch at the end of this mail), but see this
> occasional errors, doesn't happen always. Any clues on what am I missing here ?
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffcbfff700 (LWP 11301)]
> 0x000055555597156b in event_notifier_set (e=0xa4)
>     at util/event_notifier-posix.c:97
> 97	        ret = write(e->wfd, &value, sizeof(value));
> (gdb) bt
> #0  0x000055555597156b in event_notifier_set (e=0xa4)
>     at util/event_notifier-posix.c:97
> #1  0x00005555555dd3cd in aio_notify (ctx=0x0) at async.c:233
> #2  0x00005555555dd00f in qemu_bh_schedule (bh=0x7fff300008e0) at async.c:128
> #3  0x00005555556002da in gluster_finish_aiocb (fd=0x5555562ae5d0, ret=4096, 
>     arg=0x7fffd00419c0) at block/gluster.c:409
> #4  0x00007ffff5e70cdc in glfs_io_async_cbk (ret=4096, frame=0x0, data=
>     0x555556443ee0) at glfs-fops.c:567
> #5  0x00007ffff5c2843e in synctask_wrap (old_task=0x555556365940)
>     at syncop.c:133
> #6  0x00007ffff3b03370 in ?? () from /lib64/libc.so.6
> #7  0x0000000000000000 in ?? ()
> (gdb) up
> #1  0x00005555555dd3cd in aio_notify (ctx=0x0) at async.c:233
> 233	    event_notifier_set(&ctx->notifier);
> (gdb) up
> #2  0x00005555555dd00f in qemu_bh_schedule (bh=0x7fff300008e0) at async.c:128
> 128	    aio_notify(bh->ctx);
> (gdb) p *bh
> $1 = {ctx = 0x0, cb = 0x5555555ffdcd <qemu_gluster_aio_bh>, opaque = 
>     0x7fffd00419c0, next = 0x555556345e70, scheduled = false, idle = false, 
>   deleted = true}

This looks like a use-after-free, with bh->ctx corrupted when freeing
the bottom half.  But it's not at all obvious how it can happen.

I suggest using MALLOC_PERTURB_=42 to check this theory (if it is
correct, most fields will be something like 0x2a2a2a2a2a2a2a2a).  But I
don't see anything clearly wrong in the patch... Thus perhaps it is
simpler to just remove the unreachable error handling code.

Paolo

> 
> 
> gluster: Use BH mechanism for AIO completion
> 
> Replace the existing pipe based AIO completion handing by BH based method.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  block/gluster.c |   69 ++++++-------------------------------------------------
>  1 file changed, 8 insertions(+), 61 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 46f36f8..598b335 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -30,7 +30,6 @@ typedef struct GlusterAIOCB {
>  
>  typedef struct BDRVGlusterState {
>      struct glfs *glfs;
> -    int fds[2];
>      struct glfs_fd *fd;
>      int event_reader_pos;
>      GlusterAIOCB *event_acb;
> @@ -231,12 +230,13 @@ out:
>      return NULL;
>  }
>  
> -static void qemu_gluster_complete_aio(GlusterAIOCB *acb, BDRVGlusterState *s)
> +static void qemu_gluster_aio_bh(void *opaque)
>  {
>      int ret;
> +    GlusterAIOCB *acb = opaque;
>      bool *finished = acb->finished;
>      BlockDriverCompletionFunc *cb = acb->common.cb;
> -    void *opaque = acb->common.opaque;
> +    void *cb_opaque = acb->common.opaque;
>  
>      if (!acb->ret || acb->ret == acb->size) {
>          ret = 0; /* Success */
> @@ -246,33 +246,15 @@ static void qemu_gluster_complete_aio(GlusterAIOCB *acb, BDRVGlusterState *s)
>          ret = -EIO; /* Partial read/write - fail it */
>      }
>  
> +    qemu_bh_delete(acb->bh);
>      qemu_aio_release(acb);
> -    cb(opaque, ret);
> +
> +    cb(cb_opaque, ret);
>      if (finished) {
>          *finished = true;
>      }
>  }
>  
> -static void qemu_gluster_aio_event_reader(void *opaque)
> -{
> -    BDRVGlusterState *s = opaque;
> -    ssize_t ret;
> -
> -    do {
> -        char *p = (char *)&s->event_acb;
> -
> -        ret = read(s->fds[GLUSTER_FD_READ], p + s->event_reader_pos,
> -                   sizeof(s->event_acb) - s->event_reader_pos);
> -        if (ret > 0) {
> -            s->event_reader_pos += ret;
> -            if (s->event_reader_pos == sizeof(s->event_acb)) {
> -                s->event_reader_pos = 0;
> -                qemu_gluster_complete_aio(s->event_acb, s);
> -            }
> -        }
> -    } while (ret < 0 && errno == EINTR);
> -}
> -
>  /* TODO Convert to fine grained options */
>  static QemuOptsList runtime_opts = {
>      .name = "gluster",
> @@ -329,17 +311,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      s->fd = glfs_open(s->glfs, gconf->image, open_flags);
>      if (!s->fd) {
>          ret = -errno;
> -        goto out;
> -    }
> -
> -    ret = qemu_pipe(s->fds);
> -    if (ret < 0) {
> -        ret = -errno;
> -        goto out;
>      }
> -    fcntl(s->fds[GLUSTER_FD_READ], F_SETFL, O_NONBLOCK);
> -    qemu_aio_set_fd_handler(s->fds[GLUSTER_FD_READ],
> -        qemu_gluster_aio_event_reader, NULL, s);
>  
>  out:
>      qemu_opts_del(opts);
> @@ -417,31 +389,10 @@ static const AIOCBInfo gluster_aiocb_info = {
>  static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg)
>  {
>      GlusterAIOCB *acb = (GlusterAIOCB *)arg;
> -    BlockDriverState *bs = acb->common.bs;
> -    BDRVGlusterState *s = bs->opaque;
> -    int retval;
>  
>      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]);
> -        qemu_aio_set_fd_handler(s->fds[GLUSTER_FD_READ], NULL, NULL, NULL);
> -        bs->drv = NULL; /* Make the disk inaccessible */
> -        qemu_mutex_unlock_iothread();
> -    }
> +    acb->bh = qemu_bh_new(qemu_gluster_aio_bh, acb);
> +    qemu_bh_schedule(acb->bh);
>  }
>  
>  static BlockDriverAIOCB *qemu_gluster_aio_rw(BlockDriverState *bs,
> @@ -592,10 +543,6 @@ static void qemu_gluster_close(BlockDriverState *bs)
>  {
>      BDRVGlusterState *s = bs->opaque;
>  
> -    close(s->fds[GLUSTER_FD_READ]);
> -    close(s->fds[GLUSTER_FD_WRITE]);
> -    qemu_aio_set_fd_handler(s->fds[GLUSTER_FD_READ], NULL, NULL, NULL);
> -
>      if (s->fd) {
>          glfs_close(s->fd);
>          s->fd = NULL;
>

On Fri, Aug 23, 2013 at 09:33:21AM +0200, Paolo Bonzini wrote:
> > (gdb) p *bh
> > $1 = {ctx = 0x0, cb = 0x5555555ffdcd <qemu_gluster_aio_bh>, opaque = 
> >     0x7fffd00419c0, next = 0x555556345e70, scheduled = false, idle = false, 
> >   deleted = true}
> 
> This looks like a use-after-free, with bh->ctx corrupted when freeing
> the bottom half.  But it's not at all obvious how it can happen.
> 
> I suggest using MALLOC_PERTURB_=42 to check this theory (if it is
> correct, most fields will be something like 0x2a2a2a2a2a2a2a2a).  But I
> don't see anything clearly wrong in the patch... Thus perhaps it is
> simpler to just remove the unreachable error handling code.

(gdb) p *bh
$1 = {ctx = 0x0, cb = 0x2a2a2a2a2a2a2a2a, opaque = 0x2a2a2a2a2a2a2a2a, next = 
    0x2a2a2a2a2a2a2a2a, scheduled = false, idle = false, deleted = true}

May be as note above, I should just remove the unreachable error handling
code for now.

Regards,
Bharata.