[Qemu-devel] [PATCH] block: Fix direct use of protocols as driver for bdrv_open()

[Qemu-devel] [PATCH] block: Fix direct use of protocols as driver for bdrv_open()

[Kevin Wolf]

bdrv_open_common() implements direct use of protocols by copying the
pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
first set some fields in bs, which end up in file after the swap. When
bdrv_open() destroys file, it appears to be open, and because it isn't,
qemu could segfault while trying to close it.

Reorder the operations to return immediately in such cases so that file
is correctly detected as closed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 3cc3eb0..0ae2e93 100644
--- a/block.c
+++ b/block.c
@@ -680,6 +680,18 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     trace_bdrv_open_common(bs, filename, flags, drv->format_name);
 
+    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
+        return -ENOTSUP;
+    }
+
+    /* bdrv_open() with directly using a protocol as drv. This layer is already
+     * opened, so assign it to bs (while file becomes a closed BlockDriverState)
+     * and return immediately. */
+    if (file != NULL && drv->bdrv_file_open) {
+        bdrv_swap(file, bs);
+        return 0;
+    }
+
     bs->open_flags = flags;
     bs->buffer_alignment = 512;
 
@@ -694,15 +706,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bs->filename[0] = '\0';
     }
 
-    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
-        return -ENOTSUP;
-    }
-
-    if (file != NULL && drv->bdrv_file_open) {
-        bdrv_swap(file, bs);
-        return 0;
-    }
-
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
@@ -713,13 +716,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
-        if (file != NULL) {
-            bdrv_swap(file, bs);
-            ret = 0;
-        } else {
-            assert(drv->bdrv_parse_filename || filename != NULL);
-            ret = drv->bdrv_file_open(bs, filename, options, open_flags);
-        }
+        assert(file == NULL);
+        assert(drv->bdrv_parse_filename || filename != NULL);
+        ret = drv->bdrv_file_open(bs, filename, options, open_flags);
     } else {
         assert(file != NULL);
         bs->file = file;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: Fix direct use of protocols as driver for bdrv_open()

[Stefan Hajnoczi]

On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> bdrv_open_common() implements direct use of protocols by copying the
> pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> first set some fields in bs, which end up in file after the swap. When
> bdrv_open() destroys file, it appears to be open, and because it isn't,
> qemu could segfault while trying to close it.
> 
> Reorder the operations to return immediately in such cases so that file
> is correctly detected as closed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)

Didn't apply cleanly, please check that I resolved the conflict
correctly.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH] block: Fix direct use of protocols as driver for bdrv_open()

[Kevin Wolf]

Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben:
> On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> > bdrv_open_common() implements direct use of protocols by copying the
> > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> > first set some fields in bs, which end up in file after the swap. When
> > bdrv_open() destroys file, it appears to be open, and because it isn't,
> > qemu could segfault while trying to close it.
> > 
> > Reorder the operations to return immediately in such cases so that file
> > is correctly detected as closed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> Didn't apply cleanly, please check that I resolved the conflict
> correctly.

Sorry, my bad, this was accidentally on top of a temporary quick fix...
You conflict resolution is correct.

Kevin

Re: [Qemu-devel] [PATCH] block: Fix direct use of protocols as driver for bdrv_open()

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 12:13:40PM +0100, Kevin Wolf wrote:
> Am 28.03.2013 um 11:53 hat Stefan Hajnoczi geschrieben:
> > On Wed, Mar 27, 2013 at 05:28:18PM +0100, Kevin Wolf wrote:
> > > bdrv_open_common() implements direct use of protocols by copying the
> > > pre-opened BlockDriverStates to bs using bdrv_swap(). It did however
> > > first set some fields in bs, which end up in file after the swap. When
> > > bdrv_open() destroys file, it appears to be open, and because it isn't,
> > > qemu could segfault while trying to close it.
> > > 
> > > Reorder the operations to return immediately in such cases so that file
> > > is correctly detected as closed.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block.c | 31 +++++++++++++++----------------
> > >  1 file changed, 15 insertions(+), 16 deletions(-)
> > 
> > Didn't apply cleanly, please check that I resolved the conflict
> > correctly.
> 
> Sorry, my bad, this was accidentally on top of a temporary quick fix...
> You conflict resolution is correct.

Thanks.

Stefan