From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
"armbru@redhat.com" <armbru@redhat.com>,
"stefanha@redhat.com" <stefanha@redhat.com>,
Denis Lunev <den@virtuozzo.com>
Subject: Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
Date: Wed, 14 Aug 2019 17:03:34 +0200 [thread overview]
Message-ID: <c846caa9-2f7c-70e5-5a5c-6a41c5027895@redhat.com> (raw)
In-Reply-To: <85aa4552-1600-21aa-0407-128f63665aac@virtuozzo.com>
[-- Attachment #1.1: Type: text/plain, Size: 5531 bytes --]
On 14.08.19 11:11, Vladimir Sementsov-Ogievskiy wrote:
> 14.08.2019 0:31, Max Reitz wrote:
>> On 30.07.19 16:18, Vladimir Sementsov-Ogievskiy wrote:
>>> Further patch will run partial requests of iterations of
>>> qcow2_co_preadv in parallel for performance reasons. To prepare for
>>> this, separate part which may be parallelized into separate function
>>> (qcow2_co_preadv_task).
>>>
>>> While being here, also separate encrypted clusters reading to own
>>> function, like it is done for compressed reading.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> qapi/block-core.json | 2 +-
>>> block/qcow2.c | 206 +++++++++++++++++++++++--------------------
>>> 2 files changed, 112 insertions(+), 96 deletions(-)
>>
>> Looks good to me overall, just wondering about some details, as always.
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 93ab7edcea..7fa71968b2 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1967,17 +1967,115 @@ out:
>>> return ret;
>>> }
>>>
>>> +static coroutine_fn int
>>> +qcow2_co_preadv_encrypted(BlockDriverState *bs,
>>> + uint64_t file_cluster_offset,
>>> + uint64_t offset,
>>> + uint64_t bytes,
>>> + QEMUIOVector *qiov,
>>> + uint64_t qiov_offset)
>>> +{
>>> + int ret;
>>> + BDRVQcow2State *s = bs->opaque;
>>> + uint8_t *buf;
>>> +
>>> + assert(bs->encrypted && s->crypto);
>>> + assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
>>> +
>>> + /*
>>> + * For encrypted images, read everything into a temporary
>>> + * contiguous buffer on which the AES functions can work.
>>> + * Note, that we can implement enctyption, working on qiov,
>>
>> -, and s/enctyption/encryption/
>>
>>> + * but we must not do decryption in guest buffers for security
>>> + * reasons.
>>
>> "for security reasons" is a bit handwave-y, no?
>
> Hmm, let's think of it a bit.
>
> WRITE
>
> 1. We can't do any operations on write buffers, as guest may use them for
> something else and not prepared for their change. [thx to Den, pointed to this fact]
Yep. So we actually cannot implement encryption in the guest buffer. :-)
> READ
>
> Hmm, here otherwise, guest should not expect something meaningful in buffers until the
> end of read operation, so theoretically we may decrypt directly in guest buffer.. What is
> bad with it?
>
> 1. Making read-part different from write and implementing support of qiov for decryptin for
> little outcome (hmm, don't double allocation for reads, is it little or not? [*]).
>
> 2. Guest can read its buffers.
> So, it may see encrypted data and guess something about it. Ideally guest
> should know nothing about encryption, but on the other hand, is there any
> real damage? I don't sure..
>
> 3. Guest can modify its buffers.
> 3.1 I think there is no guarantee that guest will not modify its data before we finished
> copying to separate buffer, so what guest finally reads is not predictable anyway.
> 3.2 But, modifying during decryption may possibly lead to guest visible error
> (which will never be if we operate on separated cluster)
>
> So if we don't afraid of [2] and [3.2], and in a specific case [*] is significant, we may want
> implement decryption on guest buffers at least as an option..
> But all it looks for me like we'll never do it.
Well, I do think it would be weird from a guest perspective to see data
changing twice. I just couldn’t figure out what the security problem
might be.
> ===
>
> So, I'd rewrite my "Note" like this:
>
> Also, decryption in separate buffer is better as it hides from the guest information
> it doesn't own (about encrypted nature of virtual disk).
Sounds good.
>> [...]
>>
>>> +static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>>> + QCow2ClusterType cluster_type,
>>> + uint64_t file_cluster_offset,
>>> + uint64_t offset, uint64_t bytes,
>>> + QEMUIOVector *qiov,
>>> + size_t qiov_offset)
>>> +{
>>> + BDRVQcow2State *s = bs->opaque;
>>> + int offset_in_cluster = offset_into_cluster(s, offset);
>>> +
>>> + switch (cluster_type) {
>>
>> [...]
>>
>>> + default:
>>> + g_assert_not_reached();
>>> + /*
>>> + * QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC handled
>>> + * in qcow2_co_preadv_part
>>
>> Hmm, I’d still add them explicitly as cases and put their own
>> g_assert_not_reach() there.
>>
>>> + */
>>> + }
>>> +
>>> + g_assert_not_reached();
>>> +
>>> + return -EIO;
>>
>> Maybe abort()ing instead of g_assert_not_reach() would save you from
>> having to return here?
>>
>
> Hmm, will check. Any reason to use g_assert_not_reached() instead of abort() in "default"?
g_assert_not_reached() makes it more explicit what it’s about.
> I just kept it like it was. But it seems to be more often practice to use just abort() in
> Qemu code.
Yes, we often use abort() instead because it always has _Noreturn (and
thus saves us from such useless return statements).
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-08-14 15:04 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-30 14:18 [Qemu-devel] [PATCH v2 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
2019-07-30 14:18 ` [Qemu-devel] [PATCH v2 1/4] block: introduce aio task pool Vladimir Sementsov-Ogievskiy
2019-08-13 20:47 ` Max Reitz
2019-08-14 8:18 ` Vladimir Sementsov-Ogievskiy
2019-07-30 14:18 ` [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part Vladimir Sementsov-Ogievskiy
2019-08-13 21:31 ` Max Reitz
2019-08-14 9:11 ` Vladimir Sementsov-Ogievskiy
2019-08-14 15:03 ` Max Reitz [this message]
2019-08-14 15:15 ` Eric Blake
2019-08-14 15:58 ` Max Reitz
2019-07-30 14:18 ` [Qemu-devel] [PATCH v2 3/4] block/qcow2: refactor qcow2_co_pwritev_part Vladimir Sementsov-Ogievskiy
2019-08-14 15:55 ` Max Reitz
2019-08-14 16:23 ` Max Reitz
2019-07-30 14:18 ` [Qemu-devel] [PATCH v2 4/4] block/qcow2: introduce parallel subrequest handling in read and write Vladimir Sementsov-Ogievskiy
2019-08-14 16:24 ` Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c846caa9-2f7c-70e5-5a5c-6a41c5027895@redhat.com \
--to=mreitz@redhat.com \
--cc=armbru@redhat.com \
--cc=den@virtuozzo.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).