[PATCH v2] stream: Remove bdrv from job in .clean()

[PATCH v2] stream: Remove bdrv from job in .clean()

[Wesley Hershberger]

This is similar to bdc4c4c5e372756a5ba3fb3a61e585b02f0dd7f4
(qmp blockdev-backup). The cor_filter_bs was added to the blockjob as
the main BDS (by passing it to block_job_create), so
bdrv_cor_filter_drop doesn't actually remove it from global_bdrv_states.

This can cause races with qmp query-named-block nodes as described in
 #3149. Stacktrace:

0  bdrv_refresh_filename (bs=0x5efed72f8350)
    at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/block.c:8082
1  0x00005efea73cf9dc in bdrv_block_device_info
    (blk=0x0, bs=0x5efed72f8350, flat=true, errp=0x7ffeb829ebd8)
    at block/qapi.c:62
2  0x00005efea7391ed3 in bdrv_named_nodes_list
    (flat=<optimized out>, errp=0x7ffeb829ebd8)
    at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/block.c:6275
3  0x00005efea7471993 in qmp_query_named_block_nodes
    (has_flat=<optimized out>, flat=<optimized out>, errp=0x7ffeb829ebd8)
    at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/blockdev.c:2834
4  qmp_marshal_query_named_block_nodes
    (args=<optimized out>, ret=0x7f2b753beec0, errp=0x7f2b753beec8)
    at qapi/qapi-commands-block-core.c:553
5  0x00005efea74f03a5 in do_qmp_dispatch_bh (opaque=0x7f2b753beed0)
    at qapi/qmp-dispatch.c:128
6  0x00005efea75108e6 in aio_bh_poll (ctx=0x5efed6f3f430)
    at util/async.c:219
7  0x00005efea74ffdb2 in aio_dispatch (ctx=0x5efed6f3f430)
    at util/aio-posix.c:436
8  0x00005efea7512846 in aio_ctx_dispatch (source=<optimized out>,
    callback=<optimized out>,user_data=<optimized out>)
    at util/async.c:361
9  0x00007f2b77809bfb in ?? ()
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
10 0x00007f2b77809e70 in g_main_context_dispatch ()
    from /lib/x86_64-linux-gnu/libglib-2.0.so.0
11 0x00005efea7517228 in glib_pollfds_poll () at util/main-loop.c:287
12 os_host_main_loop_wait (timeout=0) at util/main-loop.c:310
13 main_loop_wait (nonblocking=<optimized out>) at util/main-loop.c:589
14 0x00005efea7140482 in qemu_main_loop () at system/runstate.c:905
15 0x00005efea744e4e8 in qemu_default_main (opaque=opaque@entry=0x0)
    at system/main.c:50
16 0x00005efea6e76319 in main
    (argc=<optimized out>, argv=<optimized out>)
    at system/main.c:93

As in bdc4c4c, there is no function to remove just the cor_filter_bs
from the job, so drop all the job's nodes as they are no longer needed.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3149
Buglink: https://bugs.launchpad.net/bugs/2126951
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
---
This patch fixes the issue described in Gitlab #3149. Please see the bug
for additional context & reproducer for the issue.

I'm happy to discuss alternative approaches or resubmit as needed.

`make check-block` passes locally.

A review would be greatly appreciated as a customer's production is
impacted.

First-time patch mailer so please pardon any mistakes.
---
Changes in v2:
- Added backtrace to commit message
- Link to v1: https://lore.kernel.org/qemu-devel/20251020-fix-3149-v1-1-04b2d4db5179@canonical.com
---
 block/stream.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/stream.c b/block/stream.c
index c0616b69e259bf5a9b146dadd9dbac62bfaa9f23..1733abd8f96d7847701f54a7a55d3284387b8582 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -132,6 +132,12 @@ static void stream_clean(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 
+    /*
+     * The job still holds a reference to cor_filter_bs; drop all bdrv to
+     * ensure that it is unref-ed
+     */
+    block_job_remove_all_bdrv(&s->common);
+
     if (s->cor_filter_bs) {
         bdrv_cor_filter_drop(s->cor_filter_bs);
         s->cor_filter_bs = NULL;

---
base-commit: 3a2d5612a7422732b648b46d4b934e2e54622fd6
change-id: 20251020-fix-3149-f01ae62fa53c

Best regards,
-- 
Wesley Hershberger <wesley.hershberger@canonical.com>

Re: [PATCH v2] stream: Remove bdrv from job in .clean()

[Vladimir Sementsov-Ogievskiy]

On 21.10.25 21:28, Wesley Hershberger wrote:
> This is similar to bdc4c4c5e372756a5ba3fb3a61e585b02f0dd7f4
> (qmp blockdev-backup). The cor_filter_bs was added to the blockjob as
> the main BDS (by passing it to block_job_create), so
> bdrv_cor_filter_drop doesn't actually remove it from global_bdrv_states.
> 
> This can cause races with qmp query-named-block nodes as described in

I'm not against the patch itself..

But I still don't understand, how it fixes the bug.

Why segfault happens, which pointer is invalid? And why?


>   #3149. Stacktrace:
> 
> 0  bdrv_refresh_filename (bs=0x5efed72f8350)

Aha, I start to understand.

1. assume that's exactly the cor_filter_bs (is it? it should be noted here if so)

2. As we know, at this time, it doesn't have any children.

3. But, it (most probably) has bs->implicit = true, as we do set it for cor_filter_bs,
when filter node-name is not given in job parameters. Is it the case?

And finally, in bdrv_refresh_filename(), we have

     if (bs->implicit) {
         /* For implicit nodes, just copy everything from the single child */
         child = QLIST_FIRST(&bs->children);
         assert(QLIST_NEXT(child, next) == NULL);

         pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
                 child->bs->exact_filename);

which, of course, will not work when we don't have any children.


If my assumptions match, they should be mentioned in commit message.


---

Ok, so, the commit make a work-around: avoid listing the invalid node
in query-named-block-nodes by removing it earlier (after the commit,
the node will be removed on final bdrv_unref() in bdrv_cor_filter_drop().

This doesn't seem safe: what if someone else have another reference on
the filter node? Or will have in future? The bug will raise again.

More correct would be fix the core problem: bdrv_refresh_filename()
crashes for detached implicit filters.

Could you check, will something like this fix the reproducer:


diff --git a/block.c b/block.c
index 8848e9a7ed..dfd9090cfc 100644
--- a/block.c
+++ b/block.c
@@ -8069,6 +8069,17 @@ void bdrv_refresh_filename(BlockDriverState *bs)
      if (bs->implicit) {
          /* For implicit nodes, just copy everything from the single child */
          child = QLIST_FIRST(&bs->children);
+
+        if (!child) {
+            /*
+             * The child is detached, which means that filter node
+             * is in process of removing. Keep filename and full_open_options
+             * unchanged, let query-named-block-nodes to show them,
+             * if it called during block-job clean-up process.
+             */
+            return;
+        }
+
          assert(QLIST_NEXT(child, next) == NULL);

          pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


---

Other ideas:

Maybe, when removing the filter child, we should set its bs->drv to NULL,
as filter node becomes nonfunctional.

Or, set bs->implicit to false, which will activate generic path in
bdrv_refresh_filename (I'm not sure it will work good for the detached
filter node)


---

Hmm, I also think that mentioned
bdc4c4c5e372756a "backup: Remove nodes from job in .clean()" is
at least incomplete.

In block-layer, having reference counters in bs, we can't rely on
"hmm, let's just drop this one reference, and hope, that after next
function, the node will be completely removed".

So, we either need a guarantee, that filter node is completely removed
when we detach it (OK, detaching should just fail if it doesn't lead
to complete removal), or we should be prepared to existing detached
filters, and handle them correctly.


>      at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/block.c:8082
> 1  0x00005efea73cf9dc in bdrv_block_device_info
>      (blk=0x0, bs=0x5efed72f8350, flat=true, errp=0x7ffeb829ebd8)
>      at block/qapi.c:62
> 2  0x00005efea7391ed3 in bdrv_named_nodes_list
>      (flat=<optimized out>, errp=0x7ffeb829ebd8)
>      at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/block.c:6275
> 3  0x00005efea7471993 in qmp_query_named_block_nodes
>      (has_flat=<optimized out>, flat=<optimized out>, errp=0x7ffeb829ebd8)
>      at /usr/src/qemu-1:10.1.0+ds-5ubuntu2/b/qemu/blockdev.c:2834
> 4  qmp_marshal_query_named_block_nodes
>      (args=<optimized out>, ret=0x7f2b753beec0, errp=0x7f2b753beec8)
>      at qapi/qapi-commands-block-core.c:553
> 5  0x00005efea74f03a5 in do_qmp_dispatch_bh (opaque=0x7f2b753beed0)
>      at qapi/qmp-dispatch.c:128
> 6  0x00005efea75108e6 in aio_bh_poll (ctx=0x5efed6f3f430)
>      at util/async.c:219
> 7  0x00005efea74ffdb2 in aio_dispatch (ctx=0x5efed6f3f430)
>      at util/aio-posix.c:436
> 8  0x00005efea7512846 in aio_ctx_dispatch (source=<optimized out>,
>      callback=<optimized out>,user_data=<optimized out>)
>      at util/async.c:361
> 9  0x00007f2b77809bfb in ?? ()
>      from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> 10 0x00007f2b77809e70 in g_main_context_dispatch ()
>      from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> 11 0x00005efea7517228 in glib_pollfds_poll () at util/main-loop.c:287
> 12 os_host_main_loop_wait (timeout=0) at util/main-loop.c:310
> 13 main_loop_wait (nonblocking=<optimized out>) at util/main-loop.c:589
> 14 0x00005efea7140482 in qemu_main_loop () at system/runstate.c:905
> 15 0x00005efea744e4e8 in qemu_default_main (opaque=opaque@entry=0x0)
>      at system/main.c:50
> 16 0x00005efea6e76319 in main
>      (argc=<optimized out>, argv=<optimized out>)
>      at system/main.c:93
> 
> As in bdc4c4c, there is no function to remove just the cor_filter_bs
> from the job, so drop all the job's nodes as they are no longer needed.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3149
> Buglink: https://bugs.launchpad.net/bugs/2126951
> Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
> ---
> This patch fixes the issue described in Gitlab #3149. Please see the bug
> for additional context & reproducer for the issue.
> 
> I'm happy to discuss alternative approaches or resubmit as needed.
> 
> `make check-block` passes locally.
> 
> A review would be greatly appreciated as a customer's production is
> impacted.
> 
> First-time patch mailer so please pardon any mistakes.
> ---
> Changes in v2:
> - Added backtrace to commit message
> - Link to v1: https://lore.kernel.org/qemu-devel/20251020-fix-3149-v1-1-04b2d4db5179@canonical.com
> ---
>   block/stream.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/block/stream.c b/block/stream.c
> index c0616b69e259bf5a9b146dadd9dbac62bfaa9f23..1733abd8f96d7847701f54a7a55d3284387b8582 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -132,6 +132,12 @@ static void stream_clean(Job *job)
>   {
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>   
> +    /*
> +     * The job still holds a reference to cor_filter_bs; drop all bdrv to
> +     * ensure that it is unref-ed
> +     */
> +    block_job_remove_all_bdrv(&s->common);
> +
>       if (s->cor_filter_bs) {
>           bdrv_cor_filter_drop(s->cor_filter_bs);
>           s->cor_filter_bs = NULL;
> 
> ---
> base-commit: 3a2d5612a7422732b648b46d4b934e2e54622fd6
> change-id: 20251020-fix-3149-f01ae62fa53c
> 
> Best regards,


-- 
Best regards,
Vladimir

Re: [PATCH v2] stream: Remove bdrv from job in .clean()

[Wesley Hershberger]

On Wed, Oct 22, 2025 at 1:57 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> If my assumptions match, they should be mentioned in commit message.

This is my understanding of the problem. I'll try to be more clear in
the next submission.

> Ok, so, the commit make a work-around: avoid listing the invalid node
> in query-named-block-nodes by removing it earlier (after the commit,
> the node will be removed on final bdrv_unref() in bdrv_cor_filter_drop().
>
> This doesn't seem safe: what if someone else have another reference on
> the filter node? Or will have in future? The bug will raise again.
>
> More correct would be fix the core problem: bdrv_refresh_filename()
> crashes for detached implicit filters.
>
> Could you check, will something like this fix the reproducer:

I tested this patch last week and again today:

diff --git a/block.c b/block.c
index 8848e9a7ed..a0246a65a5 100644
--- a/block.c
+++ b/block.c
@@ -8067,6 +8067,12 @@ void bdrv_refresh_filename(BlockDriverState *bs)
     }

     if (bs->implicit) {
+        /* Avoid races where an implicit node is removed from the graph but
+         * not yet unrefed/closed */
+        if (QLIST_EMPTY(&bs->children)) {
+            return;
+        }
+
         /* For implicit nodes, just copy everything from the single child */
         child = QLIST_FIRST(&bs->children);
         assert(QLIST_NEXT(child, next) == NULL);

This resolves the first segfault, but it produces another one in
bdrv_co_get_allocated_file_size. Here's the trace from
query-named-block-nodes:

qmp_query_named_block_nodes
bdrv_named_nodes_list
bdrv_block_device_info (block/qapi.c L44)
bdrv_query_image_info (block/qapi.c L336)
bdrv_do_query_node_info (block/qapi.c L238)
bdrv_get_allocated_file_size (include/block/block-io.h L90)
bdrv_co_get_allocated_file_size (block.c L6011) (called through
co_wrapper_bdrv_lock)

And the bt from gdb:
#0  bdrv_co_get_allocated_file_size (bs=<optimized out>)
    at /usr/src/qemu-1:10.1.0+ds-5ubuntu2+test8/b/qemu/block.c:6018
#1  0x0000631d078522be in bdrv_co_get_allocated_file_size_entry
    (opaque=0x7ffd375c5dd0) at block/block-gen.c:288
#2  0x0000631d07929ec2 in coroutine_trampoline
    (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:175
#3  0x0000772778862f60 in ?? ()
    at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:66 from
target:/lib/x86_64-linux-gnu/libc.so.6
#4  0x00007ffd375c5ae0 in ?? ()
#5  0x0000000000000000 in ?? ()

I need to spend a little more time getting this reproduced properly
using a debug build; it's not clear to me (yet) what is actually
causing the segfault here. I'll work on putting together another patch
to fix this too.

> Other ideas:
>
> Maybe, when removing the filter child, we should set its bs->drv to NULL,
> as filter node becomes nonfunctional.

Based on a quick look through of the code, this would cause
query-named-block-nodes to either return nothing or fail outright; it doesn't
seem like the right solution.

> Or, set bs->implicit to false, which will activate generic path in
> bdrv_refresh_filename (I'm not sure it will work good for the detached
> filter node)

This will run into the same second segfault as above; it also feels like
a workaround instead of a real fix :/

> Hmm, I also think that mentioned
> bdc4c4c5e372756a "backup: Remove nodes from job in .clean()" is
> at least incomplete.
>
> In block-layer, having reference counters in bs, we can't rely on
> "hmm, let's just drop this one reference, and hope, that after next
> function, the node will be completely removed".
>
> So, we either need a guarantee, that filter node is completely removed
> when we detach it (OK, detaching should just fail if it doesn't lead
> to complete removal), or we should be prepared to existing detached
> filters, and handle them correctly.

Thanks, this is helpful. I originally wrote off this approach due to the second
segfault but I'll revisit it. I think I'll submit a new series instead
of a v3 of this
patch unless you'd prefer a v3.

Thanks for your help!
~Wesley

Re: [PATCH v2] stream: Remove bdrv from job in .clean()

[Vladimir Sementsov-Ogievskiy]

On 23.10.25 00:34, Wesley Hershberger wrote:
> On Wed, Oct 22, 2025 at 1:57 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> If my assumptions match, they should be mentioned in commit message.
> 
> This is my understanding of the problem. I'll try to be more clear in
> the next submission.
> 
>> Ok, so, the commit make a work-around: avoid listing the invalid node
>> in query-named-block-nodes by removing it earlier (after the commit,
>> the node will be removed on final bdrv_unref() in bdrv_cor_filter_drop().
>>
>> This doesn't seem safe: what if someone else have another reference on
>> the filter node? Or will have in future? The bug will raise again.
>>
>> More correct would be fix the core problem: bdrv_refresh_filename()
>> crashes for detached implicit filters.
>>
>> Could you check, will something like this fix the reproducer:
> 
> I tested this patch last week and again today:
> 
> diff --git a/block.c b/block.c
> index 8848e9a7ed..a0246a65a5 100644
> --- a/block.c
> +++ b/block.c
> @@ -8067,6 +8067,12 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>       }
> 
>       if (bs->implicit) {
> +        /* Avoid races where an implicit node is removed from the graph but
> +         * not yet unrefed/closed */
> +        if (QLIST_EMPTY(&bs->children)) {
> +            return;
> +        }
> +
>           /* For implicit nodes, just copy everything from the single child */
>           child = QLIST_FIRST(&bs->children);
>           assert(QLIST_NEXT(child, next) == NULL);
> 
> This resolves the first segfault, but it produces another one in
> bdrv_co_get_allocated_file_size. Here's the trace from
> query-named-block-nodes:
> 
> qmp_query_named_block_nodes
> bdrv_named_nodes_list
> bdrv_block_device_info (block/qapi.c L44)
> bdrv_query_image_info (block/qapi.c L336)
> bdrv_do_query_node_info (block/qapi.c L238)
> bdrv_get_allocated_file_size (include/block/block-io.h L90)
> bdrv_co_get_allocated_file_size (block.c L6011) (called through
> co_wrapper_bdrv_lock)

I think, in bdrv_co_get_allocated_file_size() we go to

     } else if (drv->is_filter) {
         /* Filter drivers default to the size of their filtered child */
         return bdrv_co_get_allocated_file_size(bdrv_filter_bs(bs));

branch, bdrv_filter_bs(bs) returns NULL for detached filter, and then
we crash on trying to dereference NULL pointer in

int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs)
{
     BlockDriver *drv = bs->drv;

.

So, something like
--- a/block.c
+++ b/block.c
@@ -6024,6 +6024,12 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs)
           */
          return -ENOTSUP;
      } else if (drv->is_filter) {
+        BlockDriverState *filtered = bdrv_filter_bs(bs);
+
+        if (!filtered) {
+            return -ENOMEDIUM;
+        }
+
          /* Filter drivers default to the size of their filtered child */
          return bdrv_co_get_allocated_file_size(bdrv_filter_bs(bs));
      } else {

should help.

> 
> And the bt from gdb:
> #0  bdrv_co_get_allocated_file_size (bs=<optimized out>)
>      at /usr/src/qemu-1:10.1.0+ds-5ubuntu2+test8/b/qemu/block.c:6018
> #1  0x0000631d078522be in bdrv_co_get_allocated_file_size_entry
>      (opaque=0x7ffd375c5dd0) at block/block-gen.c:288
> #2  0x0000631d07929ec2 in coroutine_trampoline
>      (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:175
> #3  0x0000772778862f60 in ?? ()
>      at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:66 from
> target:/lib/x86_64-linux-gnu/libc.so.6
> #4  0x00007ffd375c5ae0 in ?? ()
> #5  0x0000000000000000 in ?? ()
> 
> I need to spend a little more time getting this reproduced properly
> using a debug build; it's not clear to me (yet) what is actually
> causing the segfault here. I'll work on putting together another patch
> to fix this too.
> 
>> Other ideas:
>>
>> Maybe, when removing the filter child, we should set its bs->drv to NULL,
>> as filter node becomes nonfunctional.
> 
> Based on a quick look through of the code, this would cause
> query-named-block-nodes to either return nothing or fail outright; it doesn't
> seem like the right solution.
> 
>> Or, set bs->implicit to false, which will activate generic path in
>> bdrv_refresh_filename (I'm not sure it will work good for the detached
>> filter node)
> 
> This will run into the same second segfault as above; it also feels like
> a workaround instead of a real fix :/
> 
>> Hmm, I also think that mentioned
>> bdc4c4c5e372756a "backup: Remove nodes from job in .clean()" is
>> at least incomplete.
>>
>> In block-layer, having reference counters in bs, we can't rely on
>> "hmm, let's just drop this one reference, and hope, that after next
>> function, the node will be completely removed".
>>
>> So, we either need a guarantee, that filter node is completely removed
>> when we detach it (OK, detaching should just fail if it doesn't lead
>> to complete removal), or we should be prepared to existing detached
>> filters, and handle them correctly.
> 
> Thanks, this is helpful. I originally wrote off this approach due to the second
> segfault but I'll revisit it. I think I'll submit a new series instead
> of a v3 of this
> patch unless you'd prefer a v3.
> 

I think, if it feasible to fix the problem in generic code, it's better than
work-around in specific job-type.

On the other hand, we already have similar block_job_remove_all_bdrv() calls
in backup and mirror, and it works well. So we can proceed with this patch too,
just add a comment, that we are avoiding invalid block graph state by having
detached filter, which leads to crashes, because generic code is unprepared
to detached filters.

-- 
Best regards,
Vladimir