| Message ID | 79f66648c685929a144396bda24d13a207131dcf.1485878688.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/31/2017 10:09 AM, Alberto Garcia wrote: > Passing a request size larger than INT_MAX to any of the I/O commands > results in an error. While 'read' and 'write' handle the error > correctly, 'aio_read' and 'aio_write' hit an assertion: > > blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed. > > The reason is that the QEMU I/O code cannot handle request sizes > larger than INT_MAX, so this patch makes qemu-io check that all values > are within range. Ideally, it would be nice to fix the block layer to allow larger requests (since we already have code to auto-fragment down to device limits, we should be able to rely on that code instead of having to duplicate artificial constraints everywhere else in the tree). But that's a bigger task, and this is a good patch in the interim. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > qemu-io-cmds.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue 31 Jan 2017 05:31:32 PM CET, Eric Blake wrote: > Ideally, it would be nice to fix the block layer to allow larger > requests (since we already have code to auto-fragment down to device > limits, we should be able to rely on that code instead of having to > duplicate artificial constraints everywhere else in the tree). But > that's a bigger task, and this is a good patch in the interim. Related question: what's the largest request than a guest can theoretically submit? Berto
On 01/31/2017 10:36 AM, Alberto Garcia wrote: > On Tue 31 Jan 2017 05:31:32 PM CET, Eric Blake wrote: > >> Ideally, it would be nice to fix the block layer to allow larger >> requests (since we already have code to auto-fragment down to device >> limits, we should be able to rely on that code instead of having to >> duplicate artificial constraints everywhere else in the tree). But >> that's a bigger task, and this is a good patch in the interim. > > Related question: what's the largest request than a guest can > theoretically submit? off_t supports up to 2^63 (not 2^64, because it is a signed type). Ideally, we should be constrained only by the disk size (as no one actually has 2^63 bytes of storage available), by using uint64_t offset AND length in all our APIs; but right now, we still have a lot of 32-bit length issues, and often signed length limiting us to 2^31 depending on the API.
On Tue 31 Jan 2017 05:41:23 PM CET, Eric Blake <eblake@redhat.com> wrote: >>> Ideally, it would be nice to fix the block layer to allow larger >>> requests (since we already have code to auto-fragment down to device >>> limits, we should be able to rely on that code instead of having to >>> duplicate artificial constraints everywhere else in the tree). But >>> that's a bigger task, and this is a good patch in the interim. >> >> Related question: what's the largest request than a guest can >> theoretically submit? > > off_t supports up to 2^63 (not 2^64, because it is a signed type). > Ideally, we should be constrained only by the disk size (as no one > actually has 2^63 bytes of storage available), by using uint64_t > offset AND length in all our APIs; but right now, we still have a lot > of 32-bit length issues, and often signed length limiting us to 2^31 > depending on the API. My question was more like: what happens if the guest submits a request with the maximum possible size? Does QEMU handle that correctly? Berto
On 31.01.2017 19:11, Alberto Garcia wrote: > On Tue 31 Jan 2017 05:41:23 PM CET, Eric Blake <eblake@redhat.com> wrote: > >>>> Ideally, it would be nice to fix the block layer to allow larger >>>> requests (since we already have code to auto-fragment down to device >>>> limits, we should be able to rely on that code instead of having to >>>> duplicate artificial constraints everywhere else in the tree). But >>>> that's a bigger task, and this is a good patch in the interim. >>> >>> Related question: what's the largest request than a guest can >>> theoretically submit? >> >> off_t supports up to 2^63 (not 2^64, because it is a signed type). >> Ideally, we should be constrained only by the disk size (as no one >> actually has 2^63 bytes of storage available), by using uint64_t >> offset AND length in all our APIs; but right now, we still have a lot >> of 32-bit length issues, and often signed length limiting us to 2^31 >> depending on the API. > > My question was more like: what happens if the guest submits a request > with the maximum possible size? Does QEMU handle that correctly? As far as I'm aware we rely on the device emulation code to split the request. Max
On 31.01.2017 17:09, Alberto Garcia wrote: > Passing a request size larger than INT_MAX to any of the I/O commands > results in an error. While 'read' and 'write' handle the error > correctly, 'aio_read' and 'aio_write' hit an assertion: > > blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed. > > The reason is that the QEMU I/O code cannot handle request sizes > larger than INT_MAX, so this patch makes qemu-io check that all values > are within range. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > qemu-io-cmds.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 95bcde1d88..d806a83076 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -388,9 +388,14 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov, > goto fail; > } > > - if (len > SIZE_MAX) { > - printf("Argument '%s' exceeds maximum size %llu\n", arg, > - (unsigned long long)SIZE_MAX); > + if (len > INT_MAX) { > + printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX); > + goto fail; > + } > + > + if (count > INT_MAX - len) { How about using BDRV_REQUEST_MAX_BYTES instead? (not yet in master, just in my block branch) Max > + printf("The total number of bytes exceed the maximum size %d\n", > + INT_MAX); > goto fail; > } > > @@ -682,9 +687,8 @@ static int read_f(BlockBackend *blk, int argc, char **argv) > if (count < 0) { > print_cvtnum_err(count, argv[optind]); > return 0; > - } else if (count > SIZE_MAX) { > - printf("length cannot exceed %" PRIu64 ", given %s\n", > - (uint64_t) SIZE_MAX, argv[optind]); > + } else if (count > INT_MAX) { > + printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]); > return 0; > } > > @@ -1004,9 +1008,8 @@ static int write_f(BlockBackend *blk, int argc, char **argv) > if (count < 0) { > print_cvtnum_err(count, argv[optind]); > return 0; > - } else if (count > SIZE_MAX) { > - printf("length cannot exceed %" PRIu64 ", given %s\n", > - (uint64_t) SIZE_MAX, argv[optind]); > + } else if (count > INT_MAX) { > + printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]); > return 0; > } > >
On Wed 01 Feb 2017 10:36:20 PM CET, Max Reitz <mreitz@redhat.com> wrote: >> + if (count > INT_MAX - len) { > > How about using BDRV_REQUEST_MAX_BYTES instead? > > (not yet in master, just in my block branch) Sounds good to me, feel free to edit my patch directly. Berto
On 31.01.2017 17:09, Alberto Garcia wrote: > Passing a request size larger than INT_MAX to any of the I/O commands > results in an error. While 'read' and 'write' handle the error > correctly, 'aio_read' and 'aio_write' hit an assertion: > > blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed. > > The reason is that the QEMU I/O code cannot handle request sizes > larger than INT_MAX, so this patch makes qemu-io check that all values > are within range. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > qemu-io-cmds.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) Thanks, applied to my block tree, with %s/INT_MAX/BDRV_REQUEST_MAX_BYTES/g: https://github.com/XanClic/qemu/commits/block (Maybe you should take a quick glance on whether all of the changes are really OK for you, the commit currently resides under https://github.com/XanClic/qemu/commit/beaf099368f91eb44ebd49e863c57bc0ff80659f ) Max
On Wed 01 Feb 2017 11:16:38 PM CET, Max Reitz <mreitz@redhat.com> wrote: > Thanks, applied to my block tree, with > %s/INT_MAX/BDRV_REQUEST_MAX_BYTES/g: I think you can use %d to print BDRV_REQUEST_MAX_BYTES, after all the definition guarantees that it won't be larger than MIN(SIZE_MAX,INT_MAX) But it's fine with me if you prefer to cast it to uint64_t, that way it'll remain valid if BDRV_REQUEST_MAX_BYTES is increased in the future. Berto
On 02.02.2017 09:52, Alberto Garcia wrote: > On Wed 01 Feb 2017 11:16:38 PM CET, Max Reitz <mreitz@redhat.com> wrote: > >> Thanks, applied to my block tree, with >> %s/INT_MAX/BDRV_REQUEST_MAX_BYTES/g: > > I think you can use %d to print BDRV_REQUEST_MAX_BYTES, after all the > definition guarantees that it won't be larger than MIN(SIZE_MAX,INT_MAX) I'm not sure what C makes of that expression. I'd think it becomes a size_t, probably, regardless of whether it would fit into an int. Max
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 95bcde1d88..d806a83076 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -388,9 +388,14 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov, goto fail; } - if (len > SIZE_MAX) { - printf("Argument '%s' exceeds maximum size %llu\n", arg, - (unsigned long long)SIZE_MAX); + if (len > INT_MAX) { + printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX); + goto fail; + } + + if (count > INT_MAX - len) { + printf("The total number of bytes exceed the maximum size %d\n", + INT_MAX); goto fail; } @@ -682,9 +687,8 @@ static int read_f(BlockBackend *blk, int argc, char **argv) if (count < 0) { print_cvtnum_err(count, argv[optind]); return 0; - } else if (count > SIZE_MAX) { - printf("length cannot exceed %" PRIu64 ", given %s\n", - (uint64_t) SIZE_MAX, argv[optind]); + } else if (count > INT_MAX) { + printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]); return 0; } @@ -1004,9 +1008,8 @@ static int write_f(BlockBackend *blk, int argc, char **argv) if (count < 0) { print_cvtnum_err(count, argv[optind]); return 0; - } else if (count > SIZE_MAX) { - printf("length cannot exceed %" PRIu64 ", given %s\n", - (uint64_t) SIZE_MAX, argv[optind]); + } else if (count > INT_MAX) { + printf("length cannot exceed %d, given %s\n", INT_MAX, argv[optind]); return 0; }
Passing a request size larger than INT_MAX to any of the I/O commands results in an error. While 'read' and 'write' handle the error correctly, 'aio_read' and 'aio_write' hit an assertion: blk_aio_read_entry: Assertion `rwco->qiov->size == acb->bytes' failed. The reason is that the QEMU I/O code cannot handle request sizes larger than INT_MAX, so this patch makes qemu-io check that all values are within range. Signed-off-by: Alberto Garcia <berto@igalia.com> --- qemu-io-cmds.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)