From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Akihiko Odaki <akihiko.odaki@daynix.com>,
Dmitry Fleytman <dmitry.fleytman@gmail.com>,
Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH 1/2] util/iov: Do not assert offset is in iov
Date: Wed, 8 May 2024 16:51:47 +0200 [thread overview]
Message-ID: <07258bb4-bc8f-4b21-a662-402d3164b68a@linaro.org> (raw)
In-Reply-To: <20240428-iov-v1-1-7b2dd601d80b@daynix.com>
ping?
On 28/4/24 13:11, Akihiko Odaki wrote:
> iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
> that the given offset fits in the iov while tolerating the specified
> number of bytes to operate with to be greater than the size of iov.
> This is inconsistent so remove the assertions.
>
> Asserting the offset fits in the iov makes sense if it is expected that
> there are other operations that process the content before the offset
> and the content is processed in order. Under this expectation, the
> offset should point to the end of bytes that are previously processed
> and fit in the iov. However, this expectation depends on the details of
> the caller, and did not hold true at least one case and required code to
> check iov_size(), which is added with commit 83ddb3dbba2e
> ("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").
>
> Adding such a check is inefficient and error-prone. These functions
> already tolerate the specified number of bytes to operate with to be
> greater than the size of iov to avoid such checks so remove the
> assertions to tolerate invalid offset as well. They return the number of
> bytes they operated with so their callers can still check the returned
> value to ensure there are sufficient space at the given offset.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> include/qemu/iov.h | 5 +++--
> util/iov.c | 5 -----
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index 63a1c01965d1..33548058d2ee 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
> * only part of data will be copied, up to the end of the iovec.
> * Number of bytes actually copied will be returned, which is
> * min(bytes, iov_size(iov)-offset)
> - * `Offset' must point to the inside of iovec.
> + * Returns 0 when `offset' points to the outside of iovec.
> */
> size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
> size_t offset, const void *buf, size_t bytes);
> @@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
> /**
> * Set data bytes pointed out by iovec `iov' of size `iov_cnt' elements,
> * starting at byte offset `start', to value `fillc', repeating it
> - * `bytes' number of times. `Offset' must point to the inside of iovec.
> + * `bytes' number of times.
> * If `bytes' is large enough, only last bytes portion of iovec,
> * up to the end of it, will be filled with the specified value.
> * Function return actual number of bytes processed, which is
> * min(size, iov_size(iov) - offset).
> + * Returns 0 when `offset' points to the outside of iovec.
> */
> size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
> size_t offset, int fillc, size_t bytes);
> diff --git a/util/iov.c b/util/iov.c
> index 7e73948f5e3d..a523b406b7f8 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
> offset -= iov[i].iov_len;
> }
> }
> - assert(offset == 0);
> return done;
> }
>
> @@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov, const unsigned int iov_cnt,
> offset -= iov[i].iov_len;
> }
> }
> - assert(offset == 0);
> return done;
> }
>
> @@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
> offset -= iov[i].iov_len;
> }
> }
> - assert(offset == 0);
> return done;
> }
>
> @@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> bytes -= len;
> offset = 0;
> }
> - assert(offset == 0);
> return j;
> }
>
> @@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
> soffset -= src_iov[i].iov_len;
> }
> }
> - assert(soffset == 0); /* offset beyond end of src */
>
> return done;
> }
>
next prev parent reply other threads:[~2024-05-08 14:52 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-28 11:11 [PATCH 0/2] util/iov: Do not assert offset is in iov Akihiko Odaki
2024-04-28 11:11 ` [PATCH 1/2] " Akihiko Odaki
2024-05-08 14:51 ` Philippe Mathieu-Daudé [this message]
2024-04-28 11:11 ` [PATCH 2/2] Revert "hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()" Akihiko Odaki
2024-04-28 19:45 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=07258bb4-bc8f-4b21-a662-402d3164b68a@linaro.org \
--to=philmd@linaro.org \
--cc=akihiko.odaki@daynix.com \
--cc=dmitry.fleytman@gmail.com \
--cc=jasowang@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).