block: use 1 MB bounce buffers for crypto instead of 16KB

[Qemu-devel] [PATCH for 2.10] block: use 1 MB bounce buffers for crypto instead of 16KB

Posted by Daniel P. Berrange 6 years, 8 months ago

Using 16KB bounce buffers creates a significant performance
penalty for I/O to encrypted volumes on storage with high
I/O latency (rotating rust & network drives), because it
triggers lots of fairly small I/O operations.

On tests with rotating rust, and cache=none|directsync,
write speed increased from 2MiB/s to 32MiB/s, on a par
with that achieved by the in-kernel luks driver.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 58ef6f2f52..207941db9a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
 }
 
 
-#define BLOCK_CRYPTO_MAX_SECTORS 32
+#define BLOCK_CRYPTO_MAX_SECTORS 2048
 
 static coroutine_fn int
 block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
-    /* Bounce buffer so we have a linear mem region for
-     * entire sector. XXX optimize so we avoid bounce
-     * buffer in case that qiov->niov == 1
+    /* Bounce buffer because we're not permitted to touch
+     * contents of qiov - it points to guest memory.
      */
     cipher_data =
         qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
@@ -464,9 +463,8 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t sector_num,
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
-    /* Bounce buffer so we have a linear mem region for
-     * entire sector. XXX optimize so we avoid bounce
-     * buffer in case that qiov->niov == 1
+    /* Bounce buffer because we're not permitted to touch
+     * contents of qiov - it points to guest memory.
      */
     cipher_data =
         qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-- 
2.13.3

Re: [Qemu-devel] [Qemu-block] [PATCH for 2.10] block: use 1 MB bounce buffers for crypto instead of 16KB

Posted by Eric Blake 6 years, 8 months ago

On 08/04/2017 05:51 AM, Daniel P. Berrange wrote:
> Using 16KB bounce buffers creates a significant performance
> penalty for I/O to encrypted volumes on storage with high
> I/O latency (rotating rust & network drives), because it
> triggers lots of fairly small I/O operations.
> 
> On tests with rotating rust, and cache=none|directsync,
> write speed increased from 2MiB/s to 32MiB/s, on a par
> with that achieved by the in-kernel luks driver.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 58ef6f2f52..207941db9a 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
>  }
>  
>  
> -#define BLOCK_CRYPTO_MAX_SECTORS 32
> +#define BLOCK_CRYPTO_MAX_SECTORS 2048
>  
>  static coroutine_fn int
>  block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> @@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
>  
>      qemu_iovec_init(&hd_qiov, qiov->niov);
>  
> -    /* Bounce buffer so we have a linear mem region for
> -     * entire sector. XXX optimize so we avoid bounce
> -     * buffer in case that qiov->niov == 1
> +    /* Bounce buffer because we're not permitted to touch
> +     * contents of qiov - it points to guest memory.

The comment updates are accurate (and in line with your other patch for
improving documentation of the callback semantics), but slightly
unrelated to the fix at hand. However, I have no problem keeping it in
the patch.

(To make sure I understand the importance of the bounce buffer: On
reads, we can't store into the buffer until we have decrypted, so that
the guest can't transiently spy on the encrypted form; on writes, we
must read from the buffer at most once before encrypting, so that the
guest can't change the buffer under our feet while we are encrypting.)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-block] [PATCH for 2.10] block: use 1 MB bounce buffers for crypto instead of 16KB

Posted by Daniel P. Berrange 6 years, 8 months ago

On Fri, Aug 04, 2017 at 07:08:54AM -0500, Eric Blake wrote:
> On 08/04/2017 05:51 AM, Daniel P. Berrange wrote:
> > Using 16KB bounce buffers creates a significant performance
> > penalty for I/O to encrypted volumes on storage with high
> > I/O latency (rotating rust & network drives), because it
> > triggers lots of fairly small I/O operations.
> > 
> > On tests with rotating rust, and cache=none|directsync,
> > write speed increased from 2MiB/s to 32MiB/s, on a par
> > with that achieved by the in-kernel luks driver.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/crypto.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 58ef6f2f52..207941db9a 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
> >  }
> >  
> >  
> > -#define BLOCK_CRYPTO_MAX_SECTORS 32
> > +#define BLOCK_CRYPTO_MAX_SECTORS 2048
> >  
> >  static coroutine_fn int
> >  block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> > @@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> >  
> >      qemu_iovec_init(&hd_qiov, qiov->niov);
> >  
> > -    /* Bounce buffer so we have a linear mem region for
> > -     * entire sector. XXX optimize so we avoid bounce
> > -     * buffer in case that qiov->niov == 1
> > +    /* Bounce buffer because we're not permitted to touch
> > +     * contents of qiov - it points to guest memory.
> 
> The comment updates are accurate (and in line with your other patch for
> improving documentation of the callback semantics), but slightly
> unrelated to the fix at hand. However, I have no problem keeping it in
> the patch.
> 
> (To make sure I understand the importance of the bounce buffer: On
> reads, we can't store into the buffer until we have decrypted, so that
> the guest can't transiently spy on the encrypted form; on writes, we
> must read from the buffer at most once before encrypting, so that the
> guest can't change the buffer under our feet while we are encrypting.)

For writes it is even more critical - if we encrypted in place, and the
guest tried to serve a later read from its cache, it'd return cipher
text instead of plain text.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH for 2.10] block: use 1 MB bounce buffers for crypto instead of 16KB

Posted by Stefan Hajnoczi 6 years, 8 months ago

On Fri, Aug 04, 2017 at 11:51:36AM +0100, Daniel P. Berrange wrote:
> Using 16KB bounce buffers creates a significant performance
> penalty for I/O to encrypted volumes on storage with high
> I/O latency (rotating rust & network drives), because it
> triggers lots of fairly small I/O operations.
> 
> On tests with rotating rust, and cache=none|directsync,
> write speed increased from 2MiB/s to 32MiB/s, on a par
> with that achieved by the in-kernel luks driver.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/crypto.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 58ef6f2f52..207941db9a 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
>  }
>  
>  
> -#define BLOCK_CRYPTO_MAX_SECTORS 32
> +#define BLOCK_CRYPTO_MAX_SECTORS 2048
>  
>  static coroutine_fn int
>  block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> @@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
>  
>      qemu_iovec_init(&hd_qiov, qiov->niov);
>  
> -    /* Bounce buffer so we have a linear mem region for
> -     * entire sector. XXX optimize so we avoid bounce
> -     * buffer in case that qiov->niov == 1
> +    /* Bounce buffer because we're not permitted to touch
> +     * contents of qiov - it points to guest memory.
>       */
>      cipher_data =
>          qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,

In the *read* case you can modify the data buffers in-place.  But the
guest might see intermediate states in its buffers - not sure whether
this could pose a security problem.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH for 2.10] block: use 1 MB bounce buffers for crypto instead of 16KB

Posted by Daniel P. Berrange 6 years, 8 months ago

On Fri, Aug 04, 2017 at 01:48:01PM +0100, Stefan Hajnoczi wrote:
> On Fri, Aug 04, 2017 at 11:51:36AM +0100, Daniel P. Berrange wrote:
> > Using 16KB bounce buffers creates a significant performance
> > penalty for I/O to encrypted volumes on storage with high
> > I/O latency (rotating rust & network drives), because it
> > triggers lots of fairly small I/O operations.
> > 
> > On tests with rotating rust, and cache=none|directsync,
> > write speed increased from 2MiB/s to 32MiB/s, on a par
> > with that achieved by the in-kernel luks driver.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/crypto.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 58ef6f2f52..207941db9a 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -379,7 +379,7 @@ static void block_crypto_close(BlockDriverState *bs)
> >  }
> >  
> >  
> > -#define BLOCK_CRYPTO_MAX_SECTORS 32
> > +#define BLOCK_CRYPTO_MAX_SECTORS 2048
> >  
> >  static coroutine_fn int
> >  block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> > @@ -396,9 +396,8 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> >  
> >      qemu_iovec_init(&hd_qiov, qiov->niov);
> >  
> > -    /* Bounce buffer so we have a linear mem region for
> > -     * entire sector. XXX optimize so we avoid bounce
> > -     * buffer in case that qiov->niov == 1
> > +    /* Bounce buffer because we're not permitted to touch
> > +     * contents of qiov - it points to guest memory.
> >       */
> >      cipher_data =
> >          qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
> 
> In the *read* case you can modify the data buffers in-place.  But the
> guest might see intermediate states in its buffers - not sure whether
> this could pose a security problem.

Whether its a risk or not depends on the choice of crypto parameters, as
exposing ciphertext to the guest might make watermarking attacks easier
to perform. Probably not a problem in practice, but I prefer to err on
the side of caution since I can't be sure it is safe.

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|