| Message ID | 20190112175812.27068-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
12.01.2019 20:57, Eric Blake wrote: > We only had two callers to nbd_export_new; qemu-nbd.c always > passed a valid offset/length pair (because it already checked > the file length, to ensure that offset was in bounds), while > blockdev-nbd always passed 0/-1. Then nbd_export_new reduces > the size to a multiple of BDRV_SECTOR_SIZE (can only happen > when offset is not sector-aligned, since bdrv_getlength() > currently rounds up), which can result in offset being greater > than the enforced length, but that's not fatal (the server > rejects client requests that exceed the advertised length). > > However, I'm finding it easier to work with the code if we are > consistent on having both callers pass in a valid length, and > just assert that things are sane in nbd_export_new. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v3: new patch > --- > blockdev-nbd.c | 10 +++++++++- > nbd/server.c | 9 ++------- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index c76d5416b90..d73ac1b026a 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -146,6 +146,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > BlockDriverState *bs = NULL; > BlockBackend *on_eject_blk; > NBDExport *exp; > + int64_t len; > > if (!nbd_server) { > error_setg(errp, "NBD server not running"); > @@ -168,6 +169,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > return; > } > > + len = bdrv_getlength(bs); > + if (len < 0) { > + error_setg_errno(errp, -len, > + "Failed to determine the NBD export's length"); > + return; > + } > + > if (!has_writable) { > writable = false; > } > @@ -175,7 +183,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, > writable = false; > } > > - exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap, > + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, > writable ? 0 : NBD_FLAG_READ_ONLY, > NULL, false, on_eject_blk, errp); > if (!exp) { > diff --git a/nbd/server.c b/nbd/server.c > index e8c56607eff..c9937ccdc2a 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, > exp->name = g_strdup(name); > exp->description = g_strdup(description); > exp->nbdflags = nbdflags; > - exp->size = size < 0 ? blk_getlength(blk) : size; > - if (exp->size < 0) { > - error_setg_errno(errp, -exp->size, > - "Failed to determine the NBD export's length"); > - goto fail; > - } > - exp->size -= exp->size % BDRV_SECTOR_SIZE; > + assert(dev_offset <= size); @size is not size of the image, but size of the export, so it may be less than dev_offset (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, " > + exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); > > if (bitmap) { > BdrvDirtyBitmap *bm = NULL; >
On 1/15/19 3:44 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.01.2019 20:57, Eric Blake wrote: >> We only had two callers to nbd_export_new; qemu-nbd.c always >> passed a valid offset/length pair (because it already checked >> the file length, to ensure that offset was in bounds), while >> blockdev-nbd always passed 0/-1. Then nbd_export_new reduces >> the size to a multiple of BDRV_SECTOR_SIZE (can only happen >> when offset is not sector-aligned, since bdrv_getlength() >> currently rounds up), which can result in offset being greater >> than the enforced length, but that's not fatal (the server >> rejects client requests that exceed the advertised length). >> >> However, I'm finding it easier to work with the code if we are >> consistent on having both callers pass in a valid length, and >> just assert that things are sane in nbd_export_new. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> +++ b/nbd/server.c >> @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, >> exp->name = g_strdup(name); >> exp->description = g_strdup(description); >> exp->nbdflags = nbdflags; >> - exp->size = size < 0 ? blk_getlength(blk) : size; >> - if (exp->size < 0) { >> - error_setg_errno(errp, -exp->size, >> - "Failed to determine the NBD export's length"); >> - goto fail; >> - } >> - exp->size -= exp->size % BDRV_SECTOR_SIZE; >> + assert(dev_offset <= size); > > @size is not size of the image, but size of the export, so it may be less than dev_offset > (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, " But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass in dev_offset larger than size (it fails up front if dev_offset is out of bounds, whether from the -o command line option or from what it read from the partition header with the -P command line option).
15.01.2019 18:25, Eric Blake wrote: > On 1/15/19 3:44 AM, Vladimir Sementsov-Ogievskiy wrote: >> 12.01.2019 20:57, Eric Blake wrote: >>> We only had two callers to nbd_export_new; qemu-nbd.c always >>> passed a valid offset/length pair (because it already checked >>> the file length, to ensure that offset was in bounds), while >>> blockdev-nbd always passed 0/-1. Then nbd_export_new reduces >>> the size to a multiple of BDRV_SECTOR_SIZE (can only happen >>> when offset is not sector-aligned, since bdrv_getlength() >>> currently rounds up), which can result in offset being greater >>> than the enforced length, but that's not fatal (the server >>> rejects client requests that exceed the advertised length). >>> >>> However, I'm finding it easier to work with the code if we are >>> consistent on having both callers pass in a valid length, and >>> just assert that things are sane in nbd_export_new. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> > >>> +++ b/nbd/server.c >>> @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, >>> exp->name = g_strdup(name); >>> exp->description = g_strdup(description); >>> exp->nbdflags = nbdflags; >>> - exp->size = size < 0 ? blk_getlength(blk) : size; >>> - if (exp->size < 0) { >>> - error_setg_errno(errp, -exp->size, >>> - "Failed to determine the NBD export's length"); >>> - goto fail; >>> - } >>> - exp->size -= exp->size % BDRV_SECTOR_SIZE; >>> + assert(dev_offset <= size); >> >> @size is not size of the image, but size of the export, so it may be less than dev_offset >> (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, " > > But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass > in dev_offset larger than size (it fails up front if dev_offset is out > of bounds, whether from the -o command line option or from what it read > from the partition header with the -P command line option). > Don't follow =( Assume, image size 3M, and we have offset 2M, i.e. -o 2M. than in qemu-nbd.c, we have fd_size = blk_getlength(blk); # 3M ... fd_size -= dev_offset; # 1M ... export = nbd_export_new(bs, dev_offset, fd_size # bs, 2M, 1M in nbd_export_new: assert(dev_offset <= size); # 2M <= 1M fail.
On 1/15/19 10:26 AM, Vladimir Sementsov-Ogievskiy wrote: >>> @size is not size of the image, but size of the export, so it may be less than dev_offset >>> (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, " >> >> But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass >> in dev_offset larger than size (it fails up front if dev_offset is out >> of bounds, whether from the -o command line option or from what it read >> from the partition header with the -P command line option). >> > > Don't follow =( > > Assume, image size 3M, and we have offset 2M, i.e. -o 2M. > > than in qemu-nbd.c, we have > > fd_size = blk_getlength(blk); # 3M > ... > fd_size -= dev_offset; # 1M > ... > export = nbd_export_new(bs, dev_offset, fd_size # bs, 2M, 1M > > in nbd_export_new: > > assert(dev_offset <= size); # 2M <= 1M > > fail. Ouch, you are right. I don't need the assertion in server.c at all; because all callers pass in a validated size, but the validated size has no comparable relation to dev_offset.
On 1/15/19 10:58 AM, Eric Blake wrote: > On 1/15/19 10:26 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> @size is not size of the image, but size of the export, so it may be less than dev_offset >>>> (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, " >>> >>> But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass >>> in dev_offset larger than size (it fails up front if dev_offset is out >>> of bounds, whether from the -o command line option or from what it read >>> from the partition header with the -P command line option). >>> >> >> Don't follow =( >> >> Assume, image size 3M, and we have offset 2M, i.e. -o 2M. >> >> than in qemu-nbd.c, we have >> >> fd_size = blk_getlength(blk); # 3M >> ... >> fd_size -= dev_offset; # 1M >> ... >> export = nbd_export_new(bs, dev_offset, fd_size # bs, 2M, 1M >> >> in nbd_export_new: >> >> assert(dev_offset <= size); # 2M <= 1M >> >> fail. > > Ouch, you are right. I don't need the assertion in server.c at all; > because all callers pass in a validated size, but the validated size has > no comparable relation to dev_offset. Here's what I'm considering using instead, merely asserting that the inputs are non-negative and do not overflow 63 bits: diff --git i/nbd/server.c w/nbd/server.c index c9937ccdc2a..3ebcbddaa2c 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -1495,11 +1495,12 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, exp->refcount = 1; QTAILQ_INIT(&exp->clients); exp->blk = blk; + assert(dev_offset >= 0 && dev_offset <= INT64_MAX); exp->dev_offset = dev_offset; exp->name = g_strdup(name); exp->description = g_strdup(description); exp->nbdflags = nbdflags; - assert(dev_offset <= size); + assert(size >= 0 && size <= INT64_MAX - dev_offset); exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); if (bitmap) {
On 1/12/19 11:57 AM, Eric Blake wrote: s/qemp/qmp/ in the subject line > We only had two callers to nbd_export_new; qemu-nbd.c always > passed a valid offset/length pair (because it already checked > the file length, to ensure that offset was in bounds), while > blockdev-nbd always passed 0/-1. Then nbd_export_new reduces > the size to a multiple of BDRV_SECTOR_SIZE (can only happen > when offset is not sector-aligned, since bdrv_getlength() > currently rounds up), which can result in offset being greater > than the enforced length, but that's not fatal (the server > rejects client requests that exceed the advertised length). > > However, I'm finding it easier to work with the code if we are > consistent on having both callers pass in a valid length, and > just assert that things are sane in nbd_export_new. >
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index c76d5416b90..d73ac1b026a 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -146,6 +146,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, BlockDriverState *bs = NULL; BlockBackend *on_eject_blk; NBDExport *exp; + int64_t len; if (!nbd_server) { error_setg(errp, "NBD server not running"); @@ -168,6 +169,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, return; } + len = bdrv_getlength(bs); + if (len < 0) { + error_setg_errno(errp, -len, + "Failed to determine the NBD export's length"); + return; + } + if (!has_writable) { writable = false; } @@ -175,7 +183,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, writable = false; } - exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap, + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, writable ? 0 : NBD_FLAG_READ_ONLY, NULL, false, on_eject_blk, errp); if (!exp) { diff --git a/nbd/server.c b/nbd/server.c index e8c56607eff..c9937ccdc2a 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, exp->name = g_strdup(name); exp->description = g_strdup(description); exp->nbdflags = nbdflags; - exp->size = size < 0 ? blk_getlength(blk) : size; - if (exp->size < 0) { - error_setg_errno(errp, -exp->size, - "Failed to determine the NBD export's length"); - goto fail; - } - exp->size -= exp->size % BDRV_SECTOR_SIZE; + assert(dev_offset <= size); + exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); if (bitmap) { BdrvDirtyBitmap *bm = NULL;
We only had two callers to nbd_export_new; qemu-nbd.c always passed a valid offset/length pair (because it already checked the file length, to ensure that offset was in bounds), while blockdev-nbd always passed 0/-1. Then nbd_export_new reduces the size to a multiple of BDRV_SECTOR_SIZE (can only happen when offset is not sector-aligned, since bdrv_getlength() currently rounds up), which can result in offset being greater than the enforced length, but that's not fatal (the server rejects client requests that exceed the advertised length). However, I'm finding it easier to work with the code if we are consistent on having both callers pass in a valid length, and just assert that things are sane in nbd_export_new. Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: new patch --- blockdev-nbd.c | 10 +++++++++- nbd/server.c | 9 ++------- 2 files changed, 11 insertions(+), 8 deletions(-)