Re: [PATCH v3 2/4] block-backend: fix edge case in bdrv_next() where BDS associated to BB changes

[Fiona Ebner]

Hi Kevin,

Am 26.03.24 um 13:44 schrieb Kevin Wolf:
> Am 22.03.2024 um 10:50 hat Fiona Ebner geschrieben:
>> The old_bs variable in bdrv_next() is currently determined by looking
>> at the old block backend. However, if the block graph changes before
>> the next bdrv_next() call, it might be that the associated BDS is not
>> the same that was referenced previously. In that case, the wrong BDS
>> is unreferenced, leading to an assertion failure later:
>>
>>> bdrv_unref: Assertion `bs->refcnt > 0' failed.
> 
> Your change makes sense, but in theory it shouldn't make a difference.
> The real bug is in the caller, you can't allow graph modifications while
> iterating the list of nodes. Even if it doesn't crash (like after your
> patch), you don't have any guarantee that you will have seen every node
> that exists that the end - and maybe not even that you don't get the
> same node twice.
> 
>> In particular, this can happen in the context of bdrv_flush_all(),
>> when polling for bdrv_co_flush() in the generated co-wrapper leads to
>> a graph change (for example with a stream block job [0]).
> 
> The whole locking around this case is a bit tricky and would deserve
> some cleanup.
> 
> The basic rule for bdrv_next() callers is that they need to hold the
> graph reader lock as long as they are iterating the graph, otherwise
> it's not safe. This implies that the ref/unref pairs in it should never
> make a difference either - which is important, because at least
> releasing the last reference is forbidden while holding the graph lock.
> I intended to remove the ref/unref for bdrv_next(), but I didn't because
> I realised that the callers need to be audited first that they really
> obey the rules. You found one that would be problematic.
> 
> The thing that bdrv_flush_all() gets wrong is that it promises to follow
> the graph lock rules with GRAPH_RDLOCK_GUARD_MAINLOOP(), but then calls
> something that polls. The compiler can't catch this because bdrv_flush()
> is a co_wrapper_mixed_bdrv_rdlock. The behaviour for these functions is:
> 
> - If called outside of coroutine context, they are GRAPH_UNLOCKED
> - If called in coroutine context, they are GRAPH_RDLOCK
> 
> We should probably try harder to get rid of the mixed functions, because
> a synchronous co_wrapper_bdrv_rdlock could actually be marked
> GRAPH_UNLOCKED in the code and then the compiler could catch this case.
> 
> The fix for bdrv_flush_all() is probably to make it bdrv_co_flush_all()
> with a coroutine wrapper so that the graph lock is held for the whole
> function. Then calling bdrv_co_flush() while iterating the list is safe
> and doesn't allow concurrent graph modifications.

I attempted this now, but ran into two issues:

The first is that I had to add support for a function without arguments
to the block-coroutine-wrapper.py script. I can send this as an RFC in
any case if desired.

The second is that iotest 255 ran into an assertion failure upon QMP 'quit':

> ../block/graph-lock.c:113: bdrv_graph_wrlock: Assertion 
> `!qemu_in_coroutine()' failed.

Looking at the backtrace:

> #5  0x0000762a90cc3eb2 in __GI___assert_fail
>     (assertion=0x5afb07991e7d "!qemu_in_coroutine()", file=0x5afb07991e00 
> "../block/graph-lock.c", line=113, function=0x5afb07991f20 
> <__PRETTY_FUNCTION__.4> "bdrv_graph_wrlock")
>     at ./assert/assert.c:101
> #6  0x00005afb07585311 in bdrv_graph_wrlock () at ../block/graph-lock.c:113
> #7  0x00005afb07573a36 in blk_remove_bs (blk=0x5afb0af99420) at 
> ../block/block-backend.c:901
> #8  0x00005afb075729a7 in blk_delete (blk=0x5afb0af99420) at 
> ../block/block-backend.c:487
> #9  0x00005afb07572d88 in blk_unref (blk=0x5afb0af99420) at 
> ../block/block-backend.c:547
> #10 0x00005afb07572fe8 in bdrv_next (it=0x762a852fef00) at 
> ../block/block-backend.c:618
> #11 0x00005afb0758cd65 in bdrv_co_flush_all () at ../block/io.c:2347
> #12 0x00005afb0753ba37 in bdrv_co_flush_all_entry (opaque=0x7ffff12c6050) at 
> block/block-gen.c:1391
> #13 0x00005afb0773bf41 in coroutine_trampoline (i0=168365184, i1=23291)

So I guess calling bdrv_next() is not safe from a coroutine, because the
function doing the iteration could end up being the last thing to have a
reference for the BB.

Best Regards,
Fiona