nbdkit problem with cache/cow and unaligned sizes
Right now, the cache and cow filters always round up requests to blksize boundaries (blksize for cache is dynamically determined at connection start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the underlying file, but can cause problems when reading past EOF for a partial tail of an underlying plugin. We aren't validating that filter calls to next_ops are within bounds; and even if the plugin tolerates the past-EOF read, we aren't guaranteeing that the client will always read 0 bytes in the past-EOF tail. Several ideas of fixing it, each with some drawbacks: + in cache/cow_get_size(), truncate the plugin's size down to blksize prior to calling blk_set_size() (renders the plugin's tail unusable) + reject serving images that aren't already aligned to blksize (avoids missing bytes or worrying about past-EOF slop, but can be mean, unless...) + document that for unaligned images, you can use --filter=cache --filter=truncate round-up=BLKSIZE, to let the truncate filter take care of our slop (doesn't play nicely with the fact that we can only use a filter once, if a user wants to also use --filter=truncate prior to --filter=cache) + rewrite both the cache/blk.c and cow/blk.c handlers to pay more attention to unaligned EOF (code duplication) + teach filters.c next_ops to auto-cap filter requests into valid ranges prior to calling into the next layer (trickier than it looks, especially if we later add NBD resize extension support) + others? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 5/15/19 3:11 PM, Eric Blake wrote:
Right now, the cache and cow filters always round up requests to blksize boundaries (blksize for cache is dynamically determined at connection start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the underlying file, but can cause problems when reading past EOF for a partial tail of an underlying plugin. We aren't validating that filter calls to next_ops are within bounds; and even if the plugin tolerates the past-EOF read, we aren't guaranteeing that the client will always read 0 bytes in the past-EOF tail.
Several ideas of fixing it, each with some drawbacks: + in cache/cow_get_size(), truncate the plugin's size down to blksize prior to calling blk_set_size() (renders the plugin's tail unusable)
Given that the blocksize filter already truncates the plugin's size down...
+ reject serving images that aren't already aligned to blksize (avoids missing bytes or worrying about past-EOF slop, but can be mean, unless...) + document that for unaligned images, you can use --filter=cache --filter=truncate round-up=BLKSIZE, to let the truncate filter take care of our slop (doesn't play nicely with the fact that we can only use a filter once, if a user wants to also use --filter=truncate prior to --filter=cache)
...and the truncate filter already works for rounding images up instead of down, I'm somewhat leaning towards just better documentation of the issue and use of the truncate filter, in all three of blocksize, cache, and cow. It still doesn't help anyone that wants to run the truncate filter before, rather than after, the other filters, but that's less common.
+ rewrite both the cache/blk.c and cow/blk.c handlers to pay more attention to unaligned EOF (code duplication) + teach filters.c next_ops to auto-cap filter requests into valid ranges prior to calling into the next layer (trickier than it looks, especially if we later add NBD resize extension support) + others?
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, May 15, 2019 at 03:11:42PM -0500, Eric Blake wrote:
Right now, the cache and cow filters always round up requests to blksize boundaries (blksize for cache is dynamically determined at connection start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the underlying file, but can cause problems when reading past EOF for a partial tail of an underlying plugin. We aren't validating that filter calls to next_ops are within bounds; and even if the plugin tolerates the past-EOF read, we aren't guaranteeing that the client will always read 0 bytes in the past-EOF tail.
Several ideas of fixing it, each with some drawbacks: + in cache/cow_get_size(), truncate the plugin's size down to blksize prior to calling blk_set_size() (renders the plugin's tail unusable) + reject serving images that aren't already aligned to blksize (avoids missing bytes or worrying about past-EOF slop, but can be mean, unless...) + document that for unaligned images, you can use --filter=cache --filter=truncate round-up=BLKSIZE, to let the truncate filter take care of our slop (doesn't play nicely with the fact that we can only use a filter once, if a user wants to also use --filter=truncate prior to --filter=cache) + rewrite both the cache/blk.c and cow/blk.c handlers to pay more attention to unaligned EOF (code duplication) + teach filters.c next_ops to auto-cap filter requests into valid ranges prior to calling into the next layer (trickier than it looks, especially if we later add NBD resize extension support) + others?
So I'll just make a quick comment on this: For plugins, the server is careful to only send requests to the plugin which are within the bounds of the image. Therefore plugins can simply assume that offset/count passed in to them are correct. However if a bad filter is placed on top of a plugin it could incorrectly send bad bounds requests to the plugin, which could cause mayhem. I tried to implement checking of parameters passed between layers, and it's a lot harder than it seems (for architectural reasons). Therefore we currently rely on filters being written correctly. If seems like if the cache/cow plugins don't do this then they are buggy. But I've not had time to look at this in detail. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (2)
-
Eric Blake -
Richard W.M. Jones