| Message ID | 20230714085938.202730-1-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] block: Fix pad_request's request restriction | expand |
On Fri, Jul 14, 2023 at 10:59:38AM +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. > > In effect, this patch will not change guest-visible behavior. It is a > clean-up to tighten a condition to match what is guaranteed by our > callers, and which exists purely to show clearly why the subsequent > assertion (`assert(*bytes <= SIZE_MAX)`) is always true. > > Note there is a difference between the interfaces of > 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> > --- > v2: > - Added paragraph to the commit message to express explicitly that this > patch will not change guest-visible behavior > - (No code changes) > --- > block/io.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Thanks, applied to my block-next tree: https://gitlab.com/stefanha/qemu/commits/block-next Stefan
diff --git a/block/io.c b/block/io.c index e8293d6b26..055fcf7438 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. In effect, this patch will not change guest-visible behavior. It is a clean-up to tighten a condition to match what is guaranteed by our callers, and which exists purely to show clearly why the subsequent assertion (`assert(*bytes <= SIZE_MAX)`) is always true. Note there is a difference between the interfaces of 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> --- v2: - Added paragraph to the commit message to express explicitly that this patch will not change guest-visible behavior - (No code changes) --- block/io.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)