[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above |
Date: |
Tue, 19 Nov 2019 12:12:23 +0000 |
19.11.2019 15:02, Denis Lunev wrote:
> On 11/19/19 1:22 PM, Max Reitz wrote:
>> On 16.11.19 17:34, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> I wanted to understand, what is the real difference between
>>> bdrv_block_status_above
>>> and bdrv_is_allocated_above, IMHO bdrv_is_allocated_above should work
>>> through
>>> bdrv_block_status_above..
>>>
>>> And I found the problem: bdrv_is_allocated_above considers space after EOF
>>> as
>>> UNALLOCATED for intermediate nodes..
>>>
>>> UNALLOCATED is not about allocation at fs level, but about should we go to
>>> backing or
>>> not.. And it seems incorrect for me, as in case of short backing file,
>>> we'll read
>>> zeroes after EOF, instead of going further by backing chain.
>> Should we, though? It absolutely makes sense to me to consider post-EOF
>> space as unallocated because, well, it is as unallocated as it gets.
>>
>> So from my POV it would make more sense to fall back to the backing file
>> for post-EOF reads.
>>
>> OTOH, I don’t know whether changing that behavior would qualify as a
>> possible security issue now, because maybe someone has sensitive
>> information in the tail of some disk and then truncated the overlay so
>> as to hide it? But honestly, that seems ridiculous and I can’t imagine
>> people to do that. (It would work only for the tail, and why not just
>> write zeroes there, which works everywhere?) So in practice I don’t
>> believe that to be a problem.
>>
>> Max
>
> That seems to be wrong from my POW. Once we get block device truncated,
> it exposed that tail to the guest with all zeroes.
>
> Let us assume that we have virtual disk of length L. We create new top
> delta of
> length X (less then L) and new top delta after with length Y (more than L),
> like the following:
>
> [.........................] Y
> [........] X
> [...................] L
>
> Once the guest creates FS on state Y it relies on the fact that data from X
> to Y is all zeroes.
>
> Any operations with backing chain must keep guest content to be tha same,
> i.e. if we commit from Y to L, virtual disk content should be preserved,
> i.e.
> read as all zero even if there is some data in L from X to L.
>
> If we commit from X to Y, the range from X to L should remain all zeroes.
>
> This is especially valid for backups, which can not be changed and are
> validated by the software from time to time.
>
> Does this makes sense?
>
> Den
>
Yes, if we consider space after EOF as unallocated, incremental backups are
broken,
consider the following sequence:
starting with disk of size L
full backup -> state BASE
shrink disk to size X
incremental backup to empty qcow2 of size X, with backing to BASE -> state INC1
expand disk to size Y
incremental backup to empty qcow2 of size Y, with backing to INC1 -> state INC2
Now, if we read from backup INC2, we'll see data from BASE in range [X, L],
which
we must not see.
INC2 [.........................] Y
INC1 [........] X
BASE [...................] L
Also, this example shows, that these series fixes real use-case: merge of some
incremental backups (by commit).
--
Best regards,
Vladimir
Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above, Kevin Wolf, 2019/11/19
Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above, Kevin Wolf, 2019/11/19
Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above, Kevin Wolf, 2019/11/19