| Message ID | 20230609083316.24629-1-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix pad_request's request restriction | expand |
On 09.06.23 10:33, Hanna Czenczek wrote: > bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, > which bdrv_check_qiov_request() does not guarantee. > > bdrv_check_request32() however will guarantee this, and both of > bdrv_pad_request()'s callers (bdrv_co_preadv_part() and > bdrv_co_pwritev_part()) already run it before calling > bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call > bdrv_check_request32() without expecting error, too. > > There is one difference between bdrv_check_qiov_request() and > bdrv_check_request32(): The former takes an errp, the latter does not, > so we can no longer just pass &error_abort. Instead, we need to check > the returned value. While we do expect success (because the callers > have already run this function), an assert(ret == 0) is not much simpler > than just to return an error if it occurs, so let us handle errors by > returning them up the stack now. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a > ("block: Collapse padded I/O vecs exceeding IOV_MAX") > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > block/io.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Ping
On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote: > bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, > which bdrv_check_qiov_request() does not guarantee. > > bdrv_check_request32() however will guarantee this, and both of > bdrv_pad_request()'s callers (bdrv_co_preadv_part() and > bdrv_co_pwritev_part()) already run it before calling > bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call > bdrv_check_request32() without expecting error, too. > > There is one difference between bdrv_check_qiov_request() and > bdrv_check_request32(): The former takes an errp, the latter does not, > so we can no longer just pass &error_abort. Instead, we need to check > the returned value. While we do expect success (because the callers > have already run this function), an assert(ret == 0) is not much simpler > than just to return an error if it occurs, so let us handle errors by > returning them up the stack now. Is this patch intended to silence a Coverity warning or can this be triggered by a guest? I find this commit description and patch confusing. Instead of checking the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a 32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but really INT_MAX, because that's always smaller on host architectures that QEMU supports). Vladimir: Is this the intended use of bdrv_check_request32()? > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a > ("block: Collapse padded I/O vecs exceeding IOV_MAX") > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > block/io.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 30748f0b59..e43b4ad09b 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1710,7 +1710,11 @@ static int bdrv_pad_request(BlockDriverState *bs, > int sliced_niov; > size_t sliced_head, sliced_tail; > > - bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort); > + /* Should have been checked by the caller already */ > + ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset); > + if (ret < 0) { > + return ret; > + } > > if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) { > if (padded) { > @@ -1723,7 +1727,7 @@ static int bdrv_pad_request(BlockDriverState *bs, > &sliced_head, &sliced_tail, > &sliced_niov); > > - /* Guaranteed by bdrv_check_qiov_request() */ > + /* Guaranteed by bdrv_check_request32() */ > assert(*bytes <= SIZE_MAX); > ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov, > sliced_head, *bytes); > -- > 2.40.1 >
On 11.07.23 22:23, Stefan Hajnoczi wrote: > On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote: >> bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, >> which bdrv_check_qiov_request() does not guarantee. >> >> bdrv_check_request32() however will guarantee this, and both of >> bdrv_pad_request()'s callers (bdrv_co_preadv_part() and >> bdrv_co_pwritev_part()) already run it before calling >> bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call >> bdrv_check_request32() without expecting error, too. >> >> There is one difference between bdrv_check_qiov_request() and >> bdrv_check_request32(): The former takes an errp, the latter does not, >> so we can no longer just pass &error_abort. Instead, we need to check >> the returned value. While we do expect success (because the callers >> have already run this function), an assert(ret == 0) is not much simpler >> than just to return an error if it occurs, so let us handle errors by >> returning them up the stack now. > Is this patch intended to silence a Coverity warning or can this be > triggered by a guest? Neither. There was a Coverity warning about the `assert(*bytes <= SIZE_MAX)`, which is always true on 32-bit architectures. Regardless of Coverity, Peter inquired how bdrv_check_qiov_request() would guarantee this condition (as the comments I’ve put above the assertions say). It doesn’t, only bdrv_check_request32() does, which I was thinking of, and just confused the two. As the commit message says, all callers already run bdrv_check_request32(), so I expect this change to functionally be a no-op. (That is why the pre-patch code runs bdrv_check_qiov_request() with `&error_abort`.) > I find this commit description and patch confusing. Instead of checking > the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a > 32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but > really INT_MAX, because that's always smaller on host architectures that > QEMU supports). I preferred to use a bounds-checking function that we already use for requests, and that happens to be used to limit all I/O that ends up here in bdrv_pad_request() anyway, instead of adding a new specific limit. It doesn’t matter to me, though. The callers already ensure that everything is in bounds, so I’d be happy with anything, ranging from keeping the bare assertions with no checks beforehand, over specifically checking SIZE_MAX and returning an error then, to bdrv_check_request32(). (I thought repeating the simple bounds check that all callers already did for verbosity would be the most robust and obvious way to do it, but now I’m biting myself for not just using bare assertions annotated with “Caller must guarantee this” from the start...) Hanna > Vladimir: Is this the intended use of bdrv_check_request32()? > >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a >> ("block: Collapse padded I/O vecs exceeding IOV_MAX") >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >> --- >> block/io.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> diff --git a/block/io.c b/block/io.c >> index 30748f0b59..e43b4ad09b 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1710,7 +1710,11 @@ static int bdrv_pad_request(BlockDriverState *bs, >> int sliced_niov; >> size_t sliced_head, sliced_tail; >> >> - bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort); >> + /* Should have been checked by the caller already */ >> + ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset); >> + if (ret < 0) { >> + return ret; >> + } >> >> if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) { >> if (padded) { >> @@ -1723,7 +1727,7 @@ static int bdrv_pad_request(BlockDriverState *bs, >> &sliced_head, &sliced_tail, >> &sliced_niov); >> >> - /* Guaranteed by bdrv_check_qiov_request() */ >> + /* Guaranteed by bdrv_check_request32() */ >> assert(*bytes <= SIZE_MAX); >> ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov, >> sliced_head, *bytes); >> -- >> 2.40.1 >>
On Wed, Jul 12, 2023 at 09:41:05AM +0200, Hanna Czenczek wrote: > On 11.07.23 22:23, Stefan Hajnoczi wrote: > > On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote: > > > bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, > > > which bdrv_check_qiov_request() does not guarantee. > > > > > > bdrv_check_request32() however will guarantee this, and both of > > > bdrv_pad_request()'s callers (bdrv_co_preadv_part() and > > > bdrv_co_pwritev_part()) already run it before calling > > > bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call > > > bdrv_check_request32() without expecting error, too. > > > > > > There is one difference between bdrv_check_qiov_request() and > > > bdrv_check_request32(): The former takes an errp, the latter does not, > > > so we can no longer just pass &error_abort. Instead, we need to check > > > the returned value. While we do expect success (because the callers > > > have already run this function), an assert(ret == 0) is not much simpler > > > than just to return an error if it occurs, so let us handle errors by > > > returning them up the stack now. > > Is this patch intended to silence a Coverity warning or can this be > > triggered by a guest? > > Neither. There was a Coverity warning about the `assert(*bytes <= > SIZE_MAX)`, which is always true on 32-bit architectures. Regardless of > Coverity, Peter inquired how bdrv_check_qiov_request() would guarantee this > condition (as the comments I’ve put above the assertions say). It doesn’t, > only bdrv_check_request32() does, which I was thinking of, and just confused > the two. It's unclear to me whether this patch silences a Coverity warning or not? You said "neither", but then you acknowledged there was a Coverity warning. Maybe "was" (past-tense) means something else already fixed it but I don't see any relevant commits in the git log. > As the commit message says, all callers already run bdrv_check_request32(), > so I expect this change to functionally be a no-op. (That is why the > pre-patch code runs bdrv_check_qiov_request() with `&error_abort`.) Okay, this means a guest cannot trigger the assertion failure. Please mention the intent in the commit description: a code cleanup requested by Peter and/or a Coverity warning fix, but definitely not guest triggerable assertion failure. > > > I find this commit description and patch confusing. Instead of checking > > the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a > > 32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but > > really INT_MAX, because that's always smaller on host architectures that > > QEMU supports). > > I preferred to use a bounds-checking function that we already use for > requests, and that happens to be used to limit all I/O that ends up here in > bdrv_pad_request() anyway, instead of adding a new specific limit. > > It doesn’t matter to me, though. The callers already ensure that everything > is in bounds, so I’d be happy with anything, ranging from keeping the bare > assertions with no checks beforehand, over specifically checking SIZE_MAX > and returning an error then, to bdrv_check_request32(). > > (I thought repeating the simple bounds check that all callers already did > for verbosity would be the most robust and obvious way to do it, but now I’m > biting myself for not just using bare assertions annotated with “Caller must > guarantee this” from the start...) Okay. I looked at the code more and don't see a cleanup for the overall problem of duplicated checks and type mismatches (size_t vs int64_t) that is appropriate for this patch. I'm okay with this fix, but please clarify the intent as mentioned above. > > Hanna > > > Vladimir: Is this the intended use of bdrv_check_request32()? > > > > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > > Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a > > > ("block: Collapse padded I/O vecs exceeding IOV_MAX") > > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > > > --- > > > block/io.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > diff --git a/block/io.c b/block/io.c > > > index 30748f0b59..e43b4ad09b 100644 > > > --- a/block/io.c > > > +++ b/block/io.c > > > @@ -1710,7 +1710,11 @@ static int bdrv_pad_request(BlockDriverState *bs, > > > int sliced_niov; > > > size_t sliced_head, sliced_tail; > > > - bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort); > > > + /* Should have been checked by the caller already */ > > > + ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset); > > > + if (ret < 0) { > > > + return ret; > > > + } > > > if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) { > > > if (padded) { > > > @@ -1723,7 +1727,7 @@ static int bdrv_pad_request(BlockDriverState *bs, > > > &sliced_head, &sliced_tail, > > > &sliced_niov); > > > - /* Guaranteed by bdrv_check_qiov_request() */ > > > + /* Guaranteed by bdrv_check_request32() */ > > > assert(*bytes <= SIZE_MAX); > > > ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov, > > > sliced_head, *bytes); > > > -- > > > 2.40.1 > > > >
On 12.07.23 16:15, Stefan Hajnoczi wrote: > On Wed, Jul 12, 2023 at 09:41:05AM +0200, Hanna Czenczek wrote: >> On 11.07.23 22:23, Stefan Hajnoczi wrote: >>> On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote: >>>> bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, >>>> which bdrv_check_qiov_request() does not guarantee. >>>> >>>> bdrv_check_request32() however will guarantee this, and both of >>>> bdrv_pad_request()'s callers (bdrv_co_preadv_part() and >>>> bdrv_co_pwritev_part()) already run it before calling >>>> bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call >>>> bdrv_check_request32() without expecting error, too. >>>> >>>> There is one difference between bdrv_check_qiov_request() and >>>> bdrv_check_request32(): The former takes an errp, the latter does not, >>>> so we can no longer just pass &error_abort. Instead, we need to check >>>> the returned value. While we do expect success (because the callers >>>> have already run this function), an assert(ret == 0) is not much simpler >>>> than just to return an error if it occurs, so let us handle errors by >>>> returning them up the stack now. >>> Is this patch intended to silence a Coverity warning or can this be >>> triggered by a guest? >> Neither. There was a Coverity warning about the `assert(*bytes <= >> SIZE_MAX)`, which is always true on 32-bit architectures. Regardless of >> Coverity, Peter inquired how bdrv_check_qiov_request() would guarantee this >> condition (as the comments I’ve put above the assertions say). It doesn’t, >> only bdrv_check_request32() does, which I was thinking of, and just confused >> the two. > It's unclear to me whether this patch silences a Coverity warning or > not? You said "neither", but then you acknowledged there was a Coverity > warning. Maybe "was" (past-tense) means something else already fixed it > but I don't see any relevant commits in the git log. There was and is no fix for the Coverity warning. I have mentioned that warning because the question as to why the code uses bdrv_check_qiov_request() came in the context of discussing it (https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg01809.html). I’m not planning on fixing the Coverity warning in the code. `assert(x <= SIZE_MAX)` to me is an absolutely reasonable piece of code, even if always true (on some platforms), in fact, I find it a good thing if asserted conditions are always true, not least because then the compiler can optimize them out. I don’t think we should make it more complicated to make Coverity happier. >> As the commit message says, all callers already run bdrv_check_request32(), >> so I expect this change to functionally be a no-op. (That is why the >> pre-patch code runs bdrv_check_qiov_request() with `&error_abort`.) > Okay, this means a guest cannot trigger the assertion failure. > > Please mention the intent in the commit description: a code cleanup > requested by Peter and/or a Coverity warning fix, but definitely not > guest triggerable assertion failure. Sure! >>> I find this commit description and patch confusing. Instead of checking >>> the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a >>> 32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but >>> really INT_MAX, because that's always smaller on host architectures that >>> QEMU supports). >> I preferred to use a bounds-checking function that we already use for >> requests, and that happens to be used to limit all I/O that ends up here in >> bdrv_pad_request() anyway, instead of adding a new specific limit. >> >> It doesn’t matter to me, though. The callers already ensure that everything >> is in bounds, so I’d be happy with anything, ranging from keeping the bare >> assertions with no checks beforehand, over specifically checking SIZE_MAX >> and returning an error then, to bdrv_check_request32(). >> >> (I thought repeating the simple bounds check that all callers already did >> for verbosity would be the most robust and obvious way to do it, but now I’m >> biting myself for not just using bare assertions annotated with “Caller must >> guarantee this” from the start...) > Okay. I looked at the code more and don't see a cleanup for the overall > problem of duplicated checks and type mismatches (size_t vs int64_t) > that is appropriate for this patch. > > I'm okay with this fix, but please clarify the intent as mentioned above. I can’t quite fit these two paragraphs together. It sounds like you would rather not duplicate the call to bdrv_check_request32() in bdrv_pad_request() and just defer to the callers on that one, and also address the Coverity warning in the code (instead of just ignoring it). So would you rather have me remove the bdrv_check_request32() and/or somehow address the Coverity report in the code? Hanna
On Wed, 12 Jul 2023 at 15:50, Hanna Czenczek <hreitz@redhat.com> wrote: > There was and is no fix for the Coverity warning. I have mentioned that > warning because the question as to why the code uses > bdrv_check_qiov_request() came in the context of discussing it > (https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg01809.html). > > I’m not planning on fixing the Coverity warning in the code. `assert(x > <= SIZE_MAX)` to me is an absolutely reasonable piece of code, even if > always true (on some platforms), in fact, I find it a good thing if > asserted conditions are always true, not least because then the compiler > can optimize them out. I don’t think we should make it more complicated > to make Coverity happier. Yep, I agree on that -- Coverity is bad about asserts and other conditions that are there for one particular config or host setup and only happen to be always-true on the config it scans with. The simplest thing is to mark them as false-positives. -- PMM
On Wed, 12 Jul 2023 at 10:51, Hanna Czenczek <hreitz@redhat.com> wrote: > > On 12.07.23 16:15, Stefan Hajnoczi wrote: > > On Wed, Jul 12, 2023 at 09:41:05AM +0200, Hanna Czenczek wrote: > >> On 11.07.23 22:23, Stefan Hajnoczi wrote: > >>> On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote: > >>>> bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, > >>>> which bdrv_check_qiov_request() does not guarantee. > >>>> > >>>> bdrv_check_request32() however will guarantee this, and both of > >>>> bdrv_pad_request()'s callers (bdrv_co_preadv_part() and > >>>> bdrv_co_pwritev_part()) already run it before calling > >>>> bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call > >>>> bdrv_check_request32() without expecting error, too. > >>>> > >>>> There is one difference between bdrv_check_qiov_request() and > >>>> bdrv_check_request32(): The former takes an errp, the latter does not, > >>>> so we can no longer just pass &error_abort. Instead, we need to check > >>>> the returned value. While we do expect success (because the callers > >>>> have already run this function), an assert(ret == 0) is not much simpler > >>>> than just to return an error if it occurs, so let us handle errors by > >>>> returning them up the stack now. > >>> Is this patch intended to silence a Coverity warning or can this be > >>> triggered by a guest? > >> Neither. There was a Coverity warning about the `assert(*bytes <= > >> SIZE_MAX)`, which is always true on 32-bit architectures. Regardless of > >> Coverity, Peter inquired how bdrv_check_qiov_request() would guarantee this > >> condition (as the comments I’ve put above the assertions say). It doesn’t, > >> only bdrv_check_request32() does, which I was thinking of, and just confused > >> the two. > > It's unclear to me whether this patch silences a Coverity warning or > > not? You said "neither", but then you acknowledged there was a Coverity > > warning. Maybe "was" (past-tense) means something else already fixed it > > but I don't see any relevant commits in the git log. > > There was and is no fix for the Coverity warning. I have mentioned that > warning because the question as to why the code uses > bdrv_check_qiov_request() came in the context of discussing it > (https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg01809.html). > > I’m not planning on fixing the Coverity warning in the code. `assert(x > <= SIZE_MAX)` to me is an absolutely reasonable piece of code, even if > always true (on some platforms), in fact, I find it a good thing if > asserted conditions are always true, not least because then the compiler > can optimize them out. I don’t think we should make it more complicated > to make Coverity happier. > > >> As the commit message says, all callers already run bdrv_check_request32(), > >> so I expect this change to functionally be a no-op. (That is why the > >> pre-patch code runs bdrv_check_qiov_request() with `&error_abort`.) > > Okay, this means a guest cannot trigger the assertion failure. > > > > Please mention the intent in the commit description: a code cleanup > > requested by Peter and/or a Coverity warning fix, but definitely not > > guest triggerable assertion failure. > > Sure! > > >>> I find this commit description and patch confusing. Instead of checking > >>> the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a > >>> 32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but > >>> really INT_MAX, because that's always smaller on host architectures that > >>> QEMU supports). > >> I preferred to use a bounds-checking function that we already use for > >> requests, and that happens to be used to limit all I/O that ends up here in > >> bdrv_pad_request() anyway, instead of adding a new specific limit. > >> > >> It doesn’t matter to me, though. The callers already ensure that everything > >> is in bounds, so I’d be happy with anything, ranging from keeping the bare > >> assertions with no checks beforehand, over specifically checking SIZE_MAX > >> and returning an error then, to bdrv_check_request32(). > >> > >> (I thought repeating the simple bounds check that all callers already did > >> for verbosity would be the most robust and obvious way to do it, but now I’m > >> biting myself for not just using bare assertions annotated with “Caller must > >> guarantee this” from the start...) > > Okay. I looked at the code more and don't see a cleanup for the overall > > problem of duplicated checks and type mismatches (size_t vs int64_t) > > that is appropriate for this patch. > > > > I'm okay with this fix, but please clarify the intent as mentioned above. > > I can’t quite fit these two paragraphs together. It sounds like you > would rather not duplicate the call to bdrv_check_request32() in > bdrv_pad_request() and just defer to the callers on that one, and also > address the Coverity warning in the code (instead of just ignoring it). > So would you rather have me remove the bdrv_check_request32() and/or > somehow address the Coverity report in the code? No, your code change is fine as is. I think the overall situation with bdrv_check_request() vs bdrv_check_request32(), the mixture of size_t and int64_t (and the ensuing complications), two different constants BDRV_MAX_LENGTH and BDRV_REQUEST_MAX_BYTES, and the duplication of checks in multiple functions (not just the function you're fixing) is confusing. It would be nice to fix all that, but it's outside the scope of this patch. Stefan
diff --git a/block/io.c b/block/io.c index 30748f0b59..e43b4ad09b 100644 --- a/block/io.c +++ b/block/io.c @@ -1710,7 +1710,11 @@ static int bdrv_pad_request(BlockDriverState *bs, int sliced_niov; size_t sliced_head, sliced_tail; - bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort); + /* Should have been checked by the caller already */ + ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset); + if (ret < 0) { + return ret; + } if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) { if (padded) { @@ -1723,7 +1727,7 @@ static int bdrv_pad_request(BlockDriverState *bs, &sliced_head, &sliced_tail, &sliced_niov); - /* Guaranteed by bdrv_check_qiov_request() */ + /* Guaranteed by bdrv_check_request32() */ assert(*bytes <= SIZE_MAX); ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov, sliced_head, *bytes);
bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX, which bdrv_check_qiov_request() does not guarantee. bdrv_check_request32() however will guarantee this, and both of bdrv_pad_request()'s callers (bdrv_co_preadv_part() and bdrv_co_pwritev_part()) already run it before calling bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call bdrv_check_request32() without expecting error, too. There is one difference between bdrv_check_qiov_request() and bdrv_check_request32(): The former takes an errp, the latter does not, so we can no longer just pass &error_abort. Instead, we need to check the returned value. While we do expect success (because the callers have already run this function), an assert(ret == 0) is not much simpler than just to return an error if it occurs, so let us handle errors by returning them up the stack now. Reported-by: Peter Maydell <peter.maydell@linaro.org> Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a ("block: Collapse padded I/O vecs exceeding IOV_MAX") Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- block/io.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)