[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers |
Date: |
Thu, 19 Sep 2019 18:45:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 18.09.19 17:38, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, Max Reitz wrote:
>> Hi,
>>
>> The fix (patch 1) is pretty straightforward; patch 2 (which I need for
>> the test) may not be.
>>
>> The biggest problem with patch 2 is that you can use it to uncover where
>> our permission handling is broken. For example, devising the test case
>> (patch 4) was very difficult because I kept running into the
>> &error_abort that mirror_exit_common() passes when dropping the
>> mirror_top_bs.
>>
>> The problem is that mirror_top_bs does not take the same permissions
>> that its parent takes. Ergo using &error_abort when dropping it is
>> wrong: The parent may require more permissions that mirror_top_bs did,
>> and so dropping mirror_top_bs may fail.
>>
>> Now what’s really bad is that this cannot be fixed with our current
>> permission system. mirror_top_bs was introduced precisely so it does
>> not take CONSISTENT_READ, but can still allow parents to take it (for
>> active commits). But what if there is actually something besides the
>> mirror job that unshares CONSISTENT_READ?
>>
>>
>> Imagine this:
>>
>> mirror target BB mirror source BB
>> | |
>> v v
>> mirror_top_bs -> top -> mid -> base
>> ^
>> |
>> other_parent
>>
>> The source BB unshares CONSISTENT_READ on the base. mirror_top_bs
>> ensures that its parents can read from top even though top itself cannot
>> allow CONSISTENT_READ to be taken. So far so good.
>>
>> But what if other_parent also unshares CONSISTENT_READ? Then,
>> mirror_top_bs has no business allowing its parents to take it.
>>
>> No idea how to fix that. (I suppose mirror_top_bs would need some way
>> to verify that there is no other party that has unshared CONSISTENT_READ
>> but its associated source BB.
>
> May be we need grouped permissions?
>
> Some way to define group of children, which may unshare read permission
> for other children (out of the group), but still children in group may
> have read permission?
Hm, is that different from my idea below where one of mirror_top's
children unshares the read permission, and another is allowed to take it
still?
(The problem is always that if some BDS has a parent that unshares this
permission, this condition propagates upwards through its other parents,
and we need to keep track of who unshared it in the first place.)
> But it don't work here as we are saying about children on different
> nodes.. And propagated through backing chain permissions..
Yep.
>> In the future, we want the source BB to
>> go away and instead have the source be an immediate BdrvChild of
>> mirror_top_bs. Maybe we can then build something into the block layer
>> so that a node can only restore CONSISTENT_READ when it was that node
>> that broke it?)
>>
>>
>> Anyway. You can see something arising from this problem simply by
>> unsharing CONSISTENT_READ on the target node. (Just drop the src-perm
>> node from the test I add in patch 4.) Replacing the source with the
>> target will then work fine (because mirror_top_bs doesn’t care about
>> CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs –
>> because its parent does want CONSISTENT_READ. Thus, the &error_abort
>> aborts.
>>
>>
>> While this is a more special case, I have no idea how to fix this one
>> either.
>>
>>
>> Soo... This series just fixes one thing, and leaves another unfixed
>> because I have no idea how to fix it. Worse, it adds parameters to
>> blkdebug to actually see the problem. Do we want to let blkdebug be
>> able to crash qemu (because of a bug in qemu)?
>>
>
> blkdebug is for debugging and not used by end users like libvirt, yes?
Correct.
Max
>>
>> Max Reitz (4):
>> mirror: Do not dereference invalid pointers
>> blkdebug: Allow taking/unsharing permissions
>> iotests: Add @error to wait_until_completed
>> iotests: Add test for failing mirror complete
>>
>> qapi/block-core.json | 29 +++++++++-
>> block/blkdebug.c | 106 +++++++++++++++++++++++++++++++++-
>> block/mirror.c | 13 +++--
>> tests/qemu-iotests/041 | 44 ++++++++++++++
>> tests/qemu-iotests/041.out | 4 +-
>> tests/qemu-iotests/iotests.py | 18 ++++--
>> 6 files changed, 200 insertions(+), 14 deletions(-)
>>
>
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed, (continued)
Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers, Vladimir Sementsov-Ogievskiy, 2019/09/18
- Re: [Qemu-devel] [PATCH 0/4] mirror: Do not dereference invalid pointers,
Max Reitz <=