[Qemu-devel] [PATCH RFC v2 0/1] block: pass the right options for BlockDriver.bdrv_open

[Qemu-devel] [PATCH RFC v2 0/1] block: pass the right options for BlockDriver.bdrv_open

[Dong Jia Shi]

Trying to restore rbd image on ceph cluster from snapshot with
qemu-img could trigger a calling to raw_open with a NULL @options,
and that will lead to a failure of the snapshot applying.

[root@s8345007 ~]# gdb --args qemu-img snapshot -a snap1 rbd:test_pool/dj_image
... ...
Program received signal SIGSEGV, Segmentation fault.
0x00000000801395a8 in qdict_next_entry (qdict=0x0, first_bucket=0) at qobject/qdict.c:327
327	        if (!QLIST_EMPTY(&qdict->table[i])) {
(gdb) bt
#0  0x00000000801395a8 in qdict_next_entry (qdict=0x0, first_bucket=0) at qobject/qdict.c:327
#1  0x0000000080139626 in qdict_first (qdict=0x0) at qobject/qdict.c:340
#2  0x000000008013a00c in qdict_extract_subqdict (src=0x0, dst=0x3ffffffec50, start=0x80698260 "file.")
    at qobject/qdict.c:576
#3  0x0000000080019c26 in bdrv_open_child_bs (filename=0x0, options=0x0, bdref_key=0x8017ab38 "file", 
    parent=0x80630300, child_role=0x80176108 <child_file>, allow_none=false, errp=0x0) at block.c:2018
#4  0x0000000080019dfa in bdrv_open_child (filename=0x0, options=0x0, bdref_key=0x8017ab38 "file", 
    parent=0x80630300, child_role=0x80176108 <child_file>, allow_none=false, errp=0x0) at block.c:2065
#5  0x000000008002b9a0 in raw_open (bs=0x80630300, options=0x0, flags=8194, errp=0x0) at block/raw-format.c:387
#6  0x0000000080087516 in bdrv_snapshot_goto (bs=0x80630300, snapshot_id=0x3fffffff75c "snap1")
    at block/snapshot.c:194
#7  0x0000000080010b8c in img_snapshot (argc=4, argv=0x3fffffff4c0) at qemu-img.c:2937
#8  0x00000000800140e4 in main (argc=4, argv=0x3fffffff4c0) at qemu-img.c:4373

The problematic code is /block/snapshot.c:194:
178 int bdrv_snapshot_goto(BlockDriverState *bs,
179                        const char *snapshot_id)
180 {
181     BlockDriver *drv = bs->drv;
182     int ret, open_ret;
183 
184     if (!drv) {
185         return -ENOMEDIUM;
186     }
187     if (drv->bdrv_snapshot_goto) {
188         return drv->bdrv_snapshot_goto(bs, snapshot_id);
189     }
190 
191     if (bs->file) {
192         drv->bdrv_close(bs);
193         ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
194         open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
195         if (open_ret < 0) {
196             bdrv_unref(bs->file->bs);
197             bs->drv = NULL;
198             return open_ret;
199         }
200         return ret;
201     }
202 
203     return -ENOTSUP;
204 }

After a chat with Kevin, now my understanding is that's because
NULL @options is not a valid parameter value for the bdrv_open
callback of the BlockDriver.
We shoule prepare a @options by adding the "file" key-value pair
to the actual options that were given for the node (i.e.
bs->options), and pass it to the callback.

Dong Jia Shi (1):
  block: pass the right options for BlockDriver.bdrv_open

 block/snapshot.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.8.4

[Qemu-devel] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open

[Dong Jia Shi]

raw_open() expects the caller always passing in the right actual
@options parameter. But when trying to applying snapshot on a RBD
image, bdrv_snapshot_goto() calls raw_open() (by calling the
bdrv_open callback on the BlockDriver) with a NULL @options, and
that will result in a Segmentation fault.

For the other non-raw format drivers, it also make sense to passing
in the actual options, althought they don't trigger the problem so
far.

Let's prepare a @options by adding the "file" key-value pair to a
copy of the actual options that were given for the node (i.e.
bs->options), and pass it to the callback.

Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 block/snapshot.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index bf5c2ca..dfec139 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -27,6 +27,7 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
 
 QemuOptsList internal_snapshot_opts = {
     .name = "snapshot",
@@ -189,9 +190,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     }
 
     if (bs->file) {
+        QDict *options = qdict_clone_shallow(bs->options);
+        qdict_put(options, "file",
+                  qstring_from_str(bdrv_get_node_name(bs->file->bs)));
+
         drv->bdrv_close(bs);
         ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
-        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
+        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
+        QDECREF(options);
         if (open_ret < 0) {
             bdrv_unref(bs->file->bs);
             bs->drv = NULL;
-- 
2.8.4

Re: [Qemu-devel] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3977 bytes --]

On 27.03.2017 05:05, Dong Jia Shi wrote:
> raw_open() expects the caller always passing in the right actual
> @options parameter. But when trying to applying snapshot on a RBD
> image, bdrv_snapshot_goto() calls raw_open() (by calling the
> bdrv_open callback on the BlockDriver) with a NULL @options, and
> that will result in a Segmentation fault.
> 
> For the other non-raw format drivers, it also make sense to passing
> in the actual options, althought they don't trigger the problem so
> far.
> 
> Let's prepare a @options by adding the "file" key-value pair to a
> copy of the actual options that were given for the node (i.e.
> bs->options), and pass it to the callback.
> 
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  block/snapshot.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca..dfec139 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -27,6 +27,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qstring.h"
>  
>  QemuOptsList internal_snapshot_opts = {
>      .name = "snapshot",
> @@ -189,9 +190,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      }
>  
>      if (bs->file) {
> +        QDict *options = qdict_clone_shallow(bs->options);
> +        qdict_put(options, "file",
> +                  qstring_from_str(bdrv_get_node_name(bs->file->bs)));

I don't think you're guaranteed that "file" is a a nested QDict (if it
has been specified as a nested dict). Then you'd have both options like
"file.driver" and "file" here which will break later on.

Compare:

$ ./qemu-img snapshot -a foo
"json:{'driver':'raw','file':{'driver':'qcow2','file':{'driver':'file','filename':'foo.qcow2'}}}"
qemu-img: Cannot reference an existing block device with additional
options or a new filename

$ ./qemu-img snapshot -a foo --image-opts
driver=raw,file.driver=qcow2,file.file.driver=file,file.file.filename=foo.qcow2
qemu-img: Cannot reference an existing block device with additional
options or a new filename

By the way, afterwards both commands segfault (and I had to add a manual
error_report_err() in this function to see the error message because
normally this just passes NULL for errp...). The segfault comes from...

> +
>          drv->bdrv_close(bs);
>          ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
> +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> +        QDECREF(options);
>          if (open_ret < 0) {
>              bdrv_unref(bs->file->bs);

...here. bs->file is NULL. This is because BlockDriver.bdrv_open()
expects bs->file to be NULL and just overwrites it with the result from
bdrv_open_child().

Now if that bdrv_open_child() fails, the field becomes NULL, the old
child is leaked and the above bdrv_unref() fails.

I get that this is never supposed to happen because bdrv_open_child()
should always succeed with the node name of the existing BDS given as a
reference... but we shouldn't rely on that, I guess.


All of this is a horrible mess. It kind of worked in the past when all
of this bdrv_open()/bdrv_close() stuff was a horrible mess but now it
stands out as exceptionally ugly (and obviously broken).


Of course, we can't fix all of this for 2.9, so what I'd propose for
this patch is:

(1) Remove every option in "options" that has a "file." prefix before
the qdict_put() call.

(2) Use bdrv_unref_child(bs->file) instead of bdrv_unref(bs->file->bs).

I guess it should work with those changes. At least I hope it will.

By the way, the easiest way to do (1) is probably:

QDict *file_options;

qdict_extract_subqdict(options, &file_options, "file.");
QDECREF(file_options);


Max

>              bs->drv = NULL;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open

[Dong Jia Shi]

* Max Reitz <mreitz@redhat.com> [2017-03-27 17:27:16 +0200]:

> On 27.03.2017 05:05, Dong Jia Shi wrote:
> > raw_open() expects the caller always passing in the right actual
> > @options parameter. But when trying to applying snapshot on a RBD
> > image, bdrv_snapshot_goto() calls raw_open() (by calling the
> > bdrv_open callback on the BlockDriver) with a NULL @options, and
> > that will result in a Segmentation fault.
> > 
> > For the other non-raw format drivers, it also make sense to passing
> > in the actual options, althought they don't trigger the problem so
> > far.
> > 
> > Let's prepare a @options by adding the "file" key-value pair to a
> > copy of the actual options that were given for the node (i.e.
> > bs->options), and pass it to the callback.
> > 
> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > ---
> >  block/snapshot.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index bf5c2ca..dfec139 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -27,6 +27,7 @@
> >  #include "block/block_int.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qmp/qerror.h"
> > +#include "qapi/qmp/qstring.h"
> >  
> >  QemuOptsList internal_snapshot_opts = {
> >      .name = "snapshot",
> > @@ -189,9 +190,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> >      }
> >  
> >      if (bs->file) {
> > +        QDict *options = qdict_clone_shallow(bs->options);
> > +        qdict_put(options, "file",
> > +                  qstring_from_str(bdrv_get_node_name(bs->file->bs)));
> 
> I don't think you're guaranteed that "file" is a a nested QDict (if it
> has been specified as a nested dict). Then you'd have both options like
> "file.driver" and "file" here which will break later on.
> 
> Compare:
> 
> $ ./qemu-img snapshot -a foo
> "json:{'driver':'raw','file':{'driver':'qcow2','file':{'driver':'file','filename':'foo.qcow2'}}}"
> qemu-img: Cannot reference an existing block device with additional
> options or a new filename
> 
> $ ./qemu-img snapshot -a foo --image-opts
> driver=raw,file.driver=qcow2,file.file.driver=file,file.file.filename=foo.qcow2
> qemu-img: Cannot reference an existing block device with additional
> options or a new filename
> 
> By the way, afterwards both commands segfault (and I had to add a manual
Hi Max,

You are right. Using your commands, I can easily reproduce the segfaults
problem.

> error_report_err() in this function to see the error message because
> normally this just passes NULL for errp...). The segfault comes from...
> 
> > +
> >          drv->bdrv_close(bs);
> >          ret = bdrv_snapshot_goto(bs->file->bs, snapshot_id);
> > -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
> > +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> > +        QDECREF(options);
> >          if (open_ret < 0) {
> >              bdrv_unref(bs->file->bs);
> 
> ...here. bs->file is NULL. This is because BlockDriver.bdrv_open()
> expects bs->file to be NULL and just overwrites it with the result from
> bdrv_open_child().
> 
> Now if that bdrv_open_child() fails, the field becomes NULL, the old
> child is leaked and the above bdrv_unref() fails.
Nod.

> 
> I get that this is never supposed to happen because bdrv_open_child()
> should always succeed with the node name of the existing BDS given as a
> reference... but we shouldn't rely on that, I guess.
> 
> 
> All of this is a horrible mess. It kind of worked in the past when all
> of this bdrv_open()/bdrv_close() stuff was a horrible mess but now it
> stands out as exceptionally ugly (and obviously broken).
> 
> 
> Of course, we can't fix all of this for 2.9, so what I'd propose for
> this patch is:
I definitely will listen to your suggestions on this.

> 
> (1) Remove every option in "options" that has a "file." prefix before
> the qdict_put() call.
> 
> (2) Use bdrv_unref_child(bs->file) instead of bdrv_unref(bs->file->bs).
:> You must mean:
bdrv_unref_child(bs, bs->file);

> 
> I guess it should work with those changes. At least I hope it will.
Yes, with my test results, it works!

> 
> By the way, the easiest way to do (1) is probably:
> 
> QDict *file_options;
> 
> qdict_extract_subqdict(options, &file_options, "file.");
> QDECREF(file_options);
Cool. I adopted your code and it works well.

Thanks for these very detailed analysis and the code example.

Mind if I add a:
Suggested-by: Max Reitz <mreitz@redhat.com>
in the following version?

> 
> 
> Max
> 
> >              bs->drv = NULL;
> > 
> 
> 




-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH RFC v2 1/1] block: pass the right options for BlockDriver.bdrv_open

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

On 28.03.2017 08:39, Dong Jia Shi wrote:
> * Max Reitz <mreitz@redhat.com> [2017-03-27 17:27:16 +0200]:

[...]

>> (1) Remove every option in "options" that has a "file." prefix before
>> the qdict_put() call.
>>
>> (2) Use bdrv_unref_child(bs->file) instead of bdrv_unref(bs->file->bs).
> :> You must mean:
> bdrv_unref_child(bs, bs->file);

Yes, right. :-)

>> I guess it should work with those changes. At least I hope it will.
> Yes, with my test results, it works!
> 
>>
>> By the way, the easiest way to do (1) is probably:
>>
>> QDict *file_options;
>>
>> qdict_extract_subqdict(options, &file_options, "file.");
>> QDECREF(file_options);
> Cool. I adopted your code and it works well.
> 
> Thanks for these very detailed analysis and the code example.

No problem.

> Mind if I add a:
> Suggested-by: Max Reitz <mreitz@redhat.com>
> in the following version?

Feel free to, but it's not necessary. That was just normal reviewing work.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]