Libguestfs - May 2019 - [nbdkit PATCH 0/2] Avoid oddities with files unaligned to granularity

When using a filter that rounds up to alignment boundaries, the tail
bytes of the plugin are difficult to access correctly. Rather than
duplicating lots of code already in the truncate filter, it's easier
to just make the other filters default to rounding down and add doc
links on how to round up instead.

Eric Blake (2):
  blocksize: Lift restriction against 0-size file
  cache, cow: Round size down

 filters/blocksize/nbdkit-blocksize-filter.pod | 15 +++++++++---
 filters/cache/nbdkit-cache-filter.pod         |  9 +++++++-
 filters/cow/nbdkit-cow-filter.pod             |  8 +++++++
 filters/truncate/nbdkit-truncate-filter.pod   |  9 ++++++++
 filters/blocksize/blocksize.c                 | 23 ++++---------------
 filters/cache/cache.c                         |  8 +++++--
 filters/cow/cow.c                             |  6 ++++-
 7 files changed, 53 insertions(+), 25 deletions(-)

-- 
2.20.1

When the blocksize filter was first written, nbdkit claimed that
0-size files were not supported. Later, we added the zero plugin and
ensured that a 0-size file works, so the restrictions against rounding
down to zero size are no longer necessary.  The next patch will
document how to access the trailing bytes after all.

While at it, utilize the ROUND_DOWN() macro rather than open-coding
things, as that macro also post-dated the filter's initial
implementation.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/blocksize/nbdkit-blocksize-filter.pod |  3 +--
 filters/blocksize/blocksize.c                 | 23 ++++---------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/filters/blocksize/nbdkit-blocksize-filter.pod
b/filters/blocksize/nbdkit-blocksize-filter.pod
index 0abed2f..3b79e28 100644
--- a/filters/blocksize/nbdkit-blocksize-filter.pod
+++ b/filters/blocksize/nbdkit-blocksize-filter.pod
@@ -38,8 +38,7 @@ read requests to alignment boundaries, performs
read-modify-write
 cycles for any unaligned head or tail of a write or zero request, and
 silently ignores any unaligned head or tail of a trim request.  The
 filter also truncates the plugin size down to an aligned value (as it
-cannot safely operate on the unaligned tail); it is an error if this
-would result in a size of 0.
+cannot safely operate on the unaligned tail).

 This parameter understands the suffix 'k' for 1024.

diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c
index 47bea96..ba5d9e7 100644
--- a/filters/blocksize/blocksize.c
+++ b/filters/blocksize/blocksize.c
@@ -138,30 +138,18 @@ blocksize_config_complete (nbdkit_next_config_complete
*next, void *nxdata)
   "maxdata=<SIZE>       Maximum size for read/write (default
64M).\n" \
   "maxlen=<SIZE>        Maximum size for trim/zero (default
4G-minblock)."

-static int
-blocksize_prepare (struct nbdkit_next_ops *next_ops, void *nxdata,
-                   void *handle)
-{
-  /* Early call to get_size to ensure it doesn't truncate to 0. */
-  int64_t size = next_ops->get_size (nxdata);
-
-  if (size == -1)
-    return -1;
-  if (size < minblock) {
-    nbdkit_error ("disk is too small for minblock size %u",
minblock);
-    return -1;
-  }
-  /* TODO: cache per-connection FUA mode? */
-  return 0;
-}
+/* TODO: Should we have a .prepare to cache per-connection FUA mode? */

+/* Round size down to avoid issues at end of file. */
 static int64_t
 blocksize_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,
                     void *handle)
 {
   int64_t size = next_ops->get_size (nxdata);

-  return size == -1 ? size : size & ~(minblock - 1);
+  if (size == -1)
+    return -1;
+  return ROUND_DOWN (size, minblock);
 }

 static int
@@ -387,7 +375,6 @@ static struct nbdkit_filter filter = {
   .config            = blocksize_config,
   .config_complete   = blocksize_config_complete,
   .config_help       = blocksize_config_help,
-  .prepare           = blocksize_prepare,
   .get_size          = blocksize_get_size,
   .can_multi_conn    = blocksize_can_multi_conn,
   .pread             = blocksize_pread,
-- 
2.20.1

The blocksize filter already rounds sizes down, to avoid having to
special-case a partial action at the end of an unaligned file.  But
our current behavior for cache and cow is not similarly careful, and
can end up causing errors by passing a too-large count to the
plugin. The bulk of our plugins are somewhat safe, in that they either
fail the pread (for example, the file plugin gets a short read due to
EOF) or populate the tail of the buffer with other data (which the
client will never see, because we at least clamp user requests), so
that the cache is only populated with untainted data. But it's hard to
argue whether there might be another plugin out there that might abort
because we violated our documented constraints, or which might leave
the tail of the buffer unpopulated on a short read while still
reporting success. Thus, I cannot rule out that our cache file might
contain uninitialized data from the heap, or even leak it back out to
the file system through the plugin or the NBD client.

The quickest fix is to just always round down, and then do a better
job of documenting how the user can round up as needed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/blocksize/nbdkit-blocksize-filter.pod | 14 ++++++++++++--
 filters/cache/nbdkit-cache-filter.pod         |  9 ++++++++-
 filters/cow/nbdkit-cow-filter.pod             |  8 ++++++++
 filters/truncate/nbdkit-truncate-filter.pod   |  9 +++++++++
 filters/cache/cache.c                         |  8 ++++++--
 filters/cow/cow.c                             |  6 +++++-
 6 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/filters/blocksize/nbdkit-blocksize-filter.pod
b/filters/blocksize/nbdkit-blocksize-filter.pod
index 3b79e28..c684f7d 100644
--- a/filters/blocksize/nbdkit-blocksize-filter.pod
+++ b/filters/blocksize/nbdkit-blocksize-filter.pod
@@ -38,7 +38,9 @@ read requests to alignment boundaries, performs
read-modify-write
 cycles for any unaligned head or tail of a write or zero request, and
 silently ignores any unaligned head or tail of a trim request.  The
 filter also truncates the plugin size down to an aligned value (as it
-cannot safely operate on the unaligned tail).
+cannot safely operate on the unaligned tail); if you need access to
+the last few bytes, you can combine the use of
+L<nbdkit-truncate-filter(1)> to round the size up instead.

 This parameter understands the suffix 'k' for 1024.

@@ -82,12 +84,20 @@ nbd plugin:

  nbdkit --filter=blocksize nbd maxdata=32M socket=/path/to/socket

+Serve a file as if it were a block device that insists on 4k
+alignment, while still allowing access to any unaligned bytes at the
+end of the file:
+
+ nbdkit --filter=blocksize --filter=truncate file /path/to/file \
+ mindata=4k round-up=4k
+
 =head1 SEE ALSO

 L<nbdkit(1)>,
 L<nbdkit-nbd-plugin(1)>,
 L<nbdkit-vddk-plugin(1)>,
-L<nbdkit-filter(3)>.
+L<nbdkit-filter(3)>,
+L<nbdkit-truncate-filter(1)>.

 =head1 AUTHORS

diff --git a/filters/cache/nbdkit-cache-filter.pod
b/filters/cache/nbdkit-cache-filter.pod
index 431a720..5993831 100644
--- a/filters/cache/nbdkit-cache-filter.pod
+++ b/filters/cache/nbdkit-cache-filter.pod
@@ -25,6 +25,12 @@ does not have effective caching, or (with
C<cache=unsafe>) to defeat
 flush requests from the client (which is unsafe and can cause data
 loss, as the name suggests).

+Note that the use of this filter rounds the image size down to a
+multiple of the caching granularity (the larger of 4096 or the
+C<f_bsize> field of L<fstatvfs(3)>), to ease the implementation. If
+you need to round the image size up instead to access the last few
+bytes, combine this filter with L<nbdkit-truncate-filter(1)>.
+
 =head1 PARAMETERS

 =over 4
@@ -136,6 +142,7 @@ environment variable before starting nbdkit.
 L<nbdkit(1)>,
 L<nbdkit-file-plugin(1)>,
 L<nbdkit-readahead-filter(1)>,
+L<nbdkit-truncate-filter(1)>,
 L<nbdkit-filter(3)>,
 L<qemu-img(1)>.

@@ -147,4 +154,4 @@ Richard W.M. Jones

 =head1 COPYRIGHT

-Copyright (C) 2018 Red Hat Inc.
+Copyright (C) 2018-2019 Red Hat Inc.
diff --git a/filters/cow/nbdkit-cow-filter.pod
b/filters/cow/nbdkit-cow-filter.pod
index f88d8ee..448f48c 100644
--- a/filters/cow/nbdkit-cow-filter.pod
+++ b/filters/cow/nbdkit-cow-filter.pod
@@ -31,6 +31,13 @@ that away.  It also allows us to create diffs, see below.
 The plugin is opened read-only (as if the I<-r> flag was passed), but
 you should B<not> pass the I<-r> flag to nbdkit.

+=item *
+
+Use of this filter rounds the image size down to a multiple of the
+caching granularity (4096), to ease the implementation. If you need to
+round the image size up instead to access the last few bytes, combine
+this filter with L<nbdkit-truncate-filter(1)>.
+
 =back

 Limitations of the filter include:
@@ -108,6 +115,7 @@ C<TMPDIR> environment variable before starting nbdkit.
 L<nbdkit(1)>,
 L<nbdkit-file-plugin(1)>,
 L<nbdkit-xz-plugin(1)>,
+L<nbdkit-truncate-filter(1)>,
 L<nbdkit-filter(3)>,
 L<qemu-img(1)>.

diff --git a/filters/truncate/nbdkit-truncate-filter.pod
b/filters/truncate/nbdkit-truncate-filter.pod
index 9eba6cd..92645c0 100644
--- a/filters/truncate/nbdkit-truncate-filter.pod
+++ b/filters/truncate/nbdkit-truncate-filter.pod
@@ -59,6 +59,12 @@ Round the size up to the next multiple of C<N> bytes. 
If the size of
 the underlying plugin is already a multiple of C<N> bytes, this has no
 effect.

+This option is useful when combined with other filters (such as
+L<nbdkit-blocksize-filter(1)>, L<nbdkit-cache-filter(1)>, or
+L<nbdkit-cow-filter(1)>) that normally round down due to a larger
+granularity, in order to access the last few bytes of a file that
+would otherwise be rendered inaccessible.
+
 This parameter is optional.

 =item B<round-down=>N
@@ -76,6 +82,9 @@ This parameter is optional.
 L<nbdkit(1)>,
 L<nbdkit-file-plugin(1)>,
 L<nbdkit-filter(3)>,
+L<nbdkit-blocksize-filter(1)>,
+L<nbdkit-cache-filter(1)>,
+L<nbdkit-cow-filter(1)>,
 L<nbdkit-offset-filter(1)>,
 L<nbdkit-partition-filter(1)>.

diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index 360458f..e215cac 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -60,6 +60,7 @@
 #include "reclaim.h"
 #include "isaligned.h"
 #include "minmax.h"
+#include "rounding.h"

 #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL

@@ -188,10 +189,12 @@ cache_config_complete (nbdkit_next_config_complete *next,
void *nxdata)
   return next (nxdata);
 }

-/* Get the file size and ensure the cache is the correct size. */
+/* Get the file size; round it down to cache granularity before
+ * setting cache size.
+ */
 static int64_t
 cache_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,
-              void *handle)
+                void *handle)
 {
   int64_t size;
   int r;
@@ -201,6 +204,7 @@ cache_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
     return -1;

   nbdkit_debug ("cache: underlying file size: %" PRIi64, size);
+  size = ROUND_DOWN (size, blksize);

   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   r = blk_set_size (size);
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 1ce5893..aa1348b 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -49,6 +49,7 @@
 #include "blk.h"
 #include "isaligned.h"
 #include "minmax.h"
+#include "rounding.h"

 #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL

@@ -80,7 +81,9 @@ cow_open (nbdkit_next_open *next, void *nxdata, int readonly)
   return NBDKIT_HANDLE_NOT_NEEDED;
 }

-/* Get the file size and ensure the overlay is the correct size. */
+/* Get the file size; round it down to overlay granularity before
+ * setting overlay size.
+ */
 static int64_t
 cow_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,
               void *handle)
@@ -93,6 +96,7 @@ cow_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,
     return -1;

   nbdkit_debug ("cow: underlying file size: %" PRIi64, size);
+  size = ROUND_DOWN (size, BLKSIZE);

   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
   r = blk_set_size (size);
-- 
2.20.1

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html