Patchwork [1/2] block: Fix too early free in multiwrite

login
register
mail settings
Submitter Kevin Wolf
Date July 1, 2010, 2:31 p.m.
Message ID <1277994718-14443-2-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/57553/
State New
Headers show

Comments

Kevin Wolf - July 1, 2010, 2:31 p.m.
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(-)
Stefan Hajnoczi - July 2, 2010, 8:10 a.m.
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
Christoph Hellwig - July 2, 2010, 9:38 a.m.
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>

Patch

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++;
         }
     }