* [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename
@ 2017-09-28 12:03 Vladimir Sementsov-Ogievskiy
2017-09-29 11:25 ` Max Reitz
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-09-28 12:03 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jcody, eblake, qemu-stable
Backing may be zero after failed bdrv_attach_child in
bdrv_set_backing_hd, which leads to SIGSEGV.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Hi all.
We have faced into this SIGSEGV because of image locking:
trying to call qemu-img commit on locked image leads to the following:
Program terminated with signal 11, Segmentation fault.
#0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400, opts=0x5616df268400)
at block/mirror.c:1203
1203 bdrv_refresh_filename(bs->backing->bs);
(gdb) bt
#0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400, opts=0x5616df268400)
at block/mirror.c:1203
#1 0x00005616ddc3d35f in bdrv_refresh_filename (bs=0x5616df9a7400) at block.c:4739
#2 0x00005616ddc3d672 in bdrv_set_backing_hd (bs=bs@entry=0x5616df9a7400,
backing_hd=backing_hd@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896a20)
at block.c:2035
#3 0x00005616ddc3dee3 in bdrv_append (bs_new=bs_new@entry=0x5616df9a7400,
bs_top=bs_top@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896ad8)
at block.c:3168
#4 0x00005616ddc84e5f in mirror_start_job (
job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000,
creation_flags=creation_flags@entry=0, target=target@entry=0x5616df262800,
replaces=replaces@entry=0x0, speed=speed@entry=0, granularity=65536,
granularity@entry=0, buf_size=16777216, buf_size@entry=0,
backing_mode=backing_mode@entry=MIRROR_LEAVE_BACKING_CHAIN,
on_source_error=on_source_error@entry=BLOCKDEV_ON_ERROR_REPORT,
on_target_error=on_target_error@entry=BLOCKDEV_ON_ERROR_REPORT,
unmap=unmap@entry=true, cb=cb@entry=0x5616ddc35470 <common_block_job_cb>,
opaque=opaque@entry=0x7ffff7896c80, errp=errp@entry=0x7ffff7896bd0,
driver=driver@entry=0x5616ddf8d100 <commit_active_job_driver>,
is_none_mode=is_none_mode@entry=false, base=base@entry=0x5616df262800,
auto_complete=auto_complete@entry=false,
filter_node_name=filter_node_name@entry=0x0) at block/mirror.c:1314
#5 0x00005616ddc87580 in commit_active_start (
job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000,
base=base@entry=0x5616df262800, creation_flags=creation_flags@entry=0,
speed=speed@entry=0, on_error=on_error@entry=BLOCKDEV_ON_ERROR_REPORT,
filter_node_name=filter_node_name@entry=0x0,
cb=cb@entry=0x5616ddc35470 <common_block_job_cb>,
opaque=opaque@entry=0x7ffff7896c80, errp=errp@entry=0x7ffff7896c78,
auto_complete=auto_complete@entry=false) at block/mirror.c:1463
#6 0x00005616ddc33a68 in img_commit (argc=<optimized out>, argv=<optimized out>)
at qemu-img.c:1013
#7 0x00005616ddc2fa79 in main (argc=4, argv=0x7ffff7896e00) at qemu-img.c:4548
(gdb) p bs->backing
$2 = (BdrvChild *) 0x0
(gdb) fr 2
#2 0x00005616ddc3d672 in bdrv_set_backing_hd (bs=bs@entry=0x5616df9a7400,
backing_hd=backing_hd@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896a20)
at block.c:2035
2035 bdrv_refresh_filename(bs);
(gdb) p *errp
$4 = (Error *) 0x5616df1c2660
(gdb) p **errp
$5 = {msg = 0x5616df2554e0 "Failed to get \"write\" lock",
err_class = ERROR_CLASS_GENERIC_ERROR,
src = 0x5616ddd267fe "block/file-posix.c",
func = 0x5616ddd26fe0 <__func__.27999> "raw_check_lock_bytes", line = 682,
hint = 0x5616df1fe520}
block/mirror.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index 6f5cb9f26c..351b80ca2c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1073,6 +1073,11 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
{
+ if (bs->backing == NULL) {
+ /* we can be here after failed bdrv_attach_child in
+ * bdrv_set_backing_hd */
+ return;
+ }
bdrv_refresh_filename(bs->backing->bs);
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->backing->bs->filename);
--
2.11.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename
2017-09-28 12:03 [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename Vladimir Sementsov-Ogievskiy
@ 2017-09-29 11:25 ` Max Reitz
2017-09-29 15:22 ` [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush Vladimir Sementsov-Ogievskiy
2017-09-29 19:46 ` [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename John Snow
2 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2017-09-29 11:25 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, jcody, eblake, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 345 bytes --]
On 2017-09-28 14:03, Vladimir Sementsov-Ogievskiy wrote:
> Backing may be zero after failed bdrv_attach_child in
> bdrv_set_backing_hd, which leads to SIGSEGV.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
Thanks, applied to my block branch:
https://github.com/XanClic/qemu/commits/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush
2017-09-28 12:03 [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename Vladimir Sementsov-Ogievskiy
2017-09-29 11:25 ` Max Reitz
@ 2017-09-29 15:22 ` Vladimir Sementsov-Ogievskiy
2017-09-29 17:25 ` Max Reitz
2017-09-29 19:46 ` [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename John Snow
2 siblings, 1 reply; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-09-29 15:22 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jcody, eblake, qemu-stable
Backing may be zero after failed bdrv_append in mirror_start_job,
which leads to SIGSEGV.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
similar SIGSEGV.
looks like (I guess by code, don't have full back-trace because of
coroutine switch on bdrv_flush):
mirror_start_job,
bdrv_append failed, backing is not set
bdrv_unref
bdrv_delete
bdrv_close
bdrv_flush
...
bdrv_mirror_top_flush
Segmentation fault on
return bdrv_co_flush(bs->backing->bs);
as bs->backing = 0
block/mirror.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index 6f5cb9f26c..f17c0d8726 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1056,6 +1056,10 @@ static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs,
static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
{
+ if (bs->backing == NULL) {
+ /* we can be here after failed bdrv_append in mirror_start_job */
+ return 0;
+ }
return bdrv_co_flush(bs->backing->bs);
}
--
2.11.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush
2017-09-29 15:22 ` [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush Vladimir Sementsov-Ogievskiy
@ 2017-09-29 17:25 ` Max Reitz
0 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2017-09-29 17:25 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, jcody, eblake, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 850 bytes --]
On 2017-09-29 17:22, Vladimir Sementsov-Ogievskiy wrote:
> Backing may be zero after failed bdrv_append in mirror_start_job,
> which leads to SIGSEGV.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> similar SIGSEGV.
> looks like (I guess by code, don't have full back-trace because of
> coroutine switch on bdrv_flush):
> mirror_start_job,
> bdrv_append failed, backing is not set
> bdrv_unref
> bdrv_delete
> bdrv_close
> bdrv_flush
> ...
> bdrv_mirror_top_flush
> Segmentation fault on
> return bdrv_co_flush(bs->backing->bs);
> as bs->backing = 0
>
> block/mirror.c | 4 ++++
> 1 file changed, 4 insertions(+)
Thanks, applied to my block branch:
https://github.com/XanClic/qemu/commits/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename
2017-09-28 12:03 [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename Vladimir Sementsov-Ogievskiy
2017-09-29 11:25 ` Max Reitz
2017-09-29 15:22 ` [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush Vladimir Sementsov-Ogievskiy
@ 2017-09-29 19:46 ` John Snow
2 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2017-09-29 19:46 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, qemu-stable, mreitz
On 09/28/2017 08:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> Backing may be zero after failed bdrv_attach_child in
> bdrv_set_backing_hd, which leads to SIGSEGV.
>
I guess in this case we trust bdrv_set_backing_hd to carry the errp
parameter back to the caller indicating that not only did we fail to set
the new backing_hd, but we've also lost the old one, as evidenced by the
nop-style return in .bdrv_refresh_filename...
so the error case should already be "handled", and this just cleans up a
segfault.
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Hi all.
>
> We have faced into this SIGSEGV because of image locking:
> trying to call qemu-img commit on locked image leads to the following:
>
> Program terminated with signal 11, Segmentation fault.
> #0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400, opts=0x5616df268400)
> at block/mirror.c:1203
> 1203 bdrv_refresh_filename(bs->backing->bs);
>
> (gdb) bt
> #0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400, opts=0x5616df268400)
> at block/mirror.c:1203
> #1 0x00005616ddc3d35f in bdrv_refresh_filename (bs=0x5616df9a7400) at block.c:4739
> #2 0x00005616ddc3d672 in bdrv_set_backing_hd (bs=bs@entry=0x5616df9a7400,
> backing_hd=backing_hd@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896a20)
> at block.c:2035
> #3 0x00005616ddc3dee3 in bdrv_append (bs_new=bs_new@entry=0x5616df9a7400,
> bs_top=bs_top@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896ad8)
> at block.c:3168
> #4 0x00005616ddc84e5f in mirror_start_job (
> job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000,
> creation_flags=creation_flags@entry=0, target=target@entry=0x5616df262800,
> replaces=replaces@entry=0x0, speed=speed@entry=0, granularity=65536,
> granularity@entry=0, buf_size=16777216, buf_size@entry=0,
> backing_mode=backing_mode@entry=MIRROR_LEAVE_BACKING_CHAIN,
> on_source_error=on_source_error@entry=BLOCKDEV_ON_ERROR_REPORT,
> on_target_error=on_target_error@entry=BLOCKDEV_ON_ERROR_REPORT,
> unmap=unmap@entry=true, cb=cb@entry=0x5616ddc35470 <common_block_job_cb>,
> opaque=opaque@entry=0x7ffff7896c80, errp=errp@entry=0x7ffff7896bd0,
> driver=driver@entry=0x5616ddf8d100 <commit_active_job_driver>,
> is_none_mode=is_none_mode@entry=false, base=base@entry=0x5616df262800,
> auto_complete=auto_complete@entry=false,
> filter_node_name=filter_node_name@entry=0x0) at block/mirror.c:1314
> #5 0x00005616ddc87580 in commit_active_start (
> job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000,
> base=base@entry=0x5616df262800, creation_flags=creation_flags@entry=0,
> speed=speed@entry=0, on_error=on_error@entry=BLOCKDEV_ON_ERROR_REPORT,
> filter_node_name=filter_node_name@entry=0x0,
> cb=cb@entry=0x5616ddc35470 <common_block_job_cb>,
> opaque=opaque@entry=0x7ffff7896c80, errp=errp@entry=0x7ffff7896c78,
> auto_complete=auto_complete@entry=false) at block/mirror.c:1463
> #6 0x00005616ddc33a68 in img_commit (argc=<optimized out>, argv=<optimized out>)
> at qemu-img.c:1013
> #7 0x00005616ddc2fa79 in main (argc=4, argv=0x7ffff7896e00) at qemu-img.c:4548
>
>
> (gdb) p bs->backing
> $2 = (BdrvChild *) 0x0
> (gdb) fr 2
> #2 0x00005616ddc3d672 in bdrv_set_backing_hd (bs=bs@entry=0x5616df9a7400,
> backing_hd=backing_hd@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896a20)
> at block.c:2035
> 2035 bdrv_refresh_filename(bs);
> (gdb) p *errp
> $4 = (Error *) 0x5616df1c2660
> (gdb) p **errp
> $5 = {msg = 0x5616df2554e0 "Failed to get \"write\" lock",
> err_class = ERROR_CLASS_GENERIC_ERROR,
> src = 0x5616ddd267fe "block/file-posix.c",
> func = 0x5616ddd26fe0 <__func__.27999> "raw_check_lock_bytes", line = 682,
> hint = 0x5616df1fe520}
>
>
> block/mirror.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 6f5cb9f26c..351b80ca2c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1073,6 +1073,11 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
>
> static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
> {
> + if (bs->backing == NULL) {
> + /* we can be here after failed bdrv_attach_child in
> + * bdrv_set_backing_hd */
> + return;
> + }
> bdrv_refresh_filename(bs->backing->bs);
> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
> bs->backing->bs->filename);
>
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-29 19:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-28 12:03 [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename Vladimir Sementsov-Ogievskiy
2017-09-29 11:25 ` Max Reitz
2017-09-29 15:22 ` [Qemu-devel] [PATCH] block/mirror: check backing in bdrv_mirror_top_flush Vladimir Sementsov-Ogievskiy
2017-09-29 17:25 ` Max Reitz
2017-09-29 19:46 ` [Qemu-devel] [Qemu-block] [PATCH] block/mirror: check backing in bdrv_mirror_top_refresh_filename John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.