Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: "fam@euphon.net" <fam@euphon.net>,
	Denis Lunev <den@virtuozzo.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Max Reitz <mreitz@redhat.com>,
	"stefanha@redhat.com" <stefanha@redhat.com>
Subject: Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above
Date: Tue, 19 Nov 2019 14:28:55 +0100	[thread overview]
Message-ID: <20191119132855.GD5910@linux.fritz.box> (raw)
In-Reply-To: <6cb1ffd6-3fa2-7828-fdf1-42d31974c85c@virtuozzo.com>

Am 19.11.2019 um 13:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.11.2019 15:20, Max Reitz wrote:
> > On 19.11.19 13:02, Denis V. 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?
> > 
> > All right then.  But then there’s the case of commit not shrinking the
> > backing file, so the guest content won’t be the same if you commit a
> > short overlay into a longer backing file.
> 
> Hmm. Isn't commit target truncated to source before operation?

Only if the target is smaller than the source.

Maybe we should change that, because I don't think it's expected that a
guest sees a larger disk, where old data reappears, after resizing
(shrinking) the active layer and then commiting it to the backing file.

Kevin