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.