[Qemu-devel] [PATCH 0/2] block: Fix multiwrite error handling

[Qemu-devel] [PATCH 0/2] block: Fix multiwrite error handling

[Kevin Wolf]

The bdrv_aio_multiwrite error handling has some bugs that lead to premature
cleanup, causing use-after-free and double free problems.

Kevin Wolf (2):
  block: Fix too early free in multiwrite
  block: Handle multiwrite errors only when all requests have completed

 block.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH 1/2] block: Fix too early free in multiwrite

[Kevin Wolf]

bdrv_aio_writev may call the callback immediately (and it will commonly do so
in error cases). If num_requests doesn't have its final value yet,
multiwrite_cb will falsely detect that all requests are completed and frees
the mcb. However, the mcb is still used by other requests that are started only
afterwards. When all requests are completed, it is freed for the second time.

Fix this by setting the right num_requests from the beginning.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index c40dd2c..9719649 100644
--- a/block.c
+++ b/block.c
@@ -2198,6 +2198,7 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
     num_reqs = multiwrite_merge(bs, reqs, num_reqs, mcb);
 
     // Run the aio requests
+    mcb->num_requests = num_reqs;
     for (i = 0; i < num_reqs; i++) {
         acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
             reqs[i].nb_sectors, multiwrite_cb, mcb);
@@ -2206,16 +2207,13 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
             // We can only fail the whole thing if no request has been
             // submitted yet. Otherwise we'll wait for the submitted AIOs to
             // complete and report the error in the callback.
-            if (mcb->num_requests == 0) {
+            if (i == 0) {
                 reqs[i].error = -EIO;
                 goto fail;
             } else {
-                mcb->num_requests++;
                 multiwrite_cb(mcb, -EIO);
                 break;
             }
-        } else {
-            mcb->num_requests++;
         }
     }
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/2] block: Handle multiwrite errors only when all requests have completed

[Kevin Wolf]

Don't try to be clever by freeing all temporary data and calling all callbacks
when the return value (an error) is certain. Doing so has at least two
important problems:

* The temporary data that is freed (qiov, possibly zero buffer) is still used
  by the requests that have not yet completed.
* Calling the callbacks for all requests in the multiwrite means for the caller
  that it may free buffers etc. which are still in use.

Just remember the error value and do the cleanup when all requests have
completed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 9719649..bff7d5a 100644
--- a/block.c
+++ b/block.c
@@ -2056,14 +2056,11 @@ static void multiwrite_cb(void *opaque, int ret)
 
     if (ret < 0 && !mcb->error) {
         mcb->error = ret;
-        multiwrite_user_cb(mcb);
     }
 
     mcb->num_requests--;
     if (mcb->num_requests == 0) {
-        if (mcb->error == 0) {
-            multiwrite_user_cb(mcb);
-        }
+        multiwrite_user_cb(mcb);
         qemu_free(mcb);
     }
 }
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 1/2] block: Fix too early free in multiwrite

[Stefan Hajnoczi]

On Thu, Jul 1, 2010 at 3:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> bdrv_aio_writev may call the callback immediately (and it will commonly do so
> in error cases). If num_requests doesn't have its final value yet,
> multiwrite_cb will falsely detect that all requests are completed and frees
> the mcb. However, the mcb is still used by other requests that are started only
> afterwards. When all requests are completed, it is freed for the second time.
>
> Fix this by setting the right num_requests from the beginning.

Looks good to me.

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index c40dd2c..9719649 100644
> --- a/block.c
> +++ b/block.c
> @@ -2198,6 +2198,7 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
>     num_reqs = multiwrite_merge(bs, reqs, num_reqs, mcb);
>
>     // Run the aio requests
> +    mcb->num_requests = num_reqs;
>     for (i = 0; i < num_reqs; i++) {
>         acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
>             reqs[i].nb_sectors, multiwrite_cb, mcb);
> @@ -2206,16 +2207,13 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
>             // We can only fail the whole thing if no request has been
>             // submitted yet. Otherwise we'll wait for the submitted AIOs to
>             // complete and report the error in the callback.
> -            if (mcb->num_requests == 0) {
> +            if (i == 0) {
>                 reqs[i].error = -EIO;
>                 goto fail;
>             } else {
> -                mcb->num_requests++;
>                 multiwrite_cb(mcb, -EIO);
>                 break;
>             }
> -        } else {
> -            mcb->num_requests++;
>         }
>     }
>
> --
> 1.6.6.1
>
>
>

Stefan

Re: [Qemu-devel] [PATCH 2/2] block: Handle multiwrite errors only when all requests have completed

[Stefan Hajnoczi]

On Thu, Jul 1, 2010 at 3:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Don't try to be clever by freeing all temporary data and calling all callbacks
> when the return value (an error) is certain. Doing so has at least two
> important problems:
>
> * The temporary data that is freed (qiov, possibly zero buffer) is still used
>  by the requests that have not yet completed.
> * Calling the callbacks for all requests in the multiwrite means for the caller
>  that it may free buffers etc. which are still in use.
>
> Just remember the error value and do the cleanup when all requests have
> completed.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9719649..bff7d5a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2056,14 +2056,11 @@ static void multiwrite_cb(void *opaque, int ret)
>
>     if (ret < 0 && !mcb->error) {
>         mcb->error = ret;
> -        multiwrite_user_cb(mcb);
>     }
>
>     mcb->num_requests--;
>     if (mcb->num_requests == 0) {
> -        if (mcb->error == 0) {
> -            multiwrite_user_cb(mcb);
> -        }
> +        multiwrite_user_cb(mcb);

I am a little confused at this point.  Here is the meat of
bdrv_aio_multiwrite() from the kevin/devel branch:

// Run the aio requests
mcb->num_requests = num_reqs;
for (i = 0; i < num_reqs; i++) {
    acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
        reqs[i].nb_sectors, multiwrite_cb, mcb);

    if (acb == NULL) {
        // We can only fail the whole thing if no request has been
        // submitted yet. Otherwise we'll wait for the submitted AIOs to
        // complete and report the error in the callback.
        if (i == 0) {
            reqs[i].error = -EIO;
            goto fail;
        } else {
            multiwrite_cb(mcb, -EIO);
            break;
        }
    }
}

If bdrv_aio_write() fails on request 2 out of 3, then
mcb->num_requests = 3 but only 2 requests were issued before breaking
the loop.

Now the 2 issued requests complete and call multiwrite_cb().  Since
was mcb->num_requests = 3, it never reaches zero and neither
multiwrite_user_cb(mcb) nor qemu_free(mcb) are ever called.

Is it safe to adjust mcb->num_requests = i before calling
multiwrite_cb() and breaking the loop in the failure case?  That way
the num->num_requests will reach zero.

I hesitate a little because previous requests could have completed,
multiwrite_cb() was called, and mcb->num_requests was decremented?
Then the value of i cannot be used for mcb->num_requests because
previous requests it counts have already completed.

Stefan

Re: [Qemu-devel] [PATCH 1/2] block: Fix too early free in multiwrite

[Christoph Hellwig]

On Thu, Jul 01, 2010 at 04:31:57PM +0200, Kevin Wolf wrote:
> bdrv_aio_writev may call the callback immediately (and it will commonly do so
> in error cases). If num_requests doesn't have its final value yet,
> multiwrite_cb will falsely detect that all requests are completed and frees
> the mcb. However, the mcb is still used by other requests that are started only
> afterwards. When all requests are completed, it is freed for the second time.
> 
> Fix this by setting the right num_requests from the beginning.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [Qemu-devel] [PATCH 2/2] block: Handle multiwrite errors only when all requests have completed

[Christoph Hellwig]

On Thu, Jul 01, 2010 at 04:31:58PM +0200, Kevin Wolf wrote:
> Don't try to be clever by freeing all temporary data and calling all callbacks
> when the return value (an error) is certain. Doing so has at least two
> important problems:
> 
> * The temporary data that is freed (qiov, possibly zero buffer) is still used
>   by the requests that have not yet completed.
> * Calling the callbacks for all requests in the multiwrite means for the caller
>   that it may free buffers etc. which are still in use.
> 
> Just remember the error value and do the cleanup when all requests have
> completed.

Looks good.