Re: [PATCH v5 03/29] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 2 Jun 2025 17:36:06 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 26/05/2025 16:19, Stefano Brivio wrote:
> > On Thu, 17 Apr 2025 18:51:10 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> Provide a temporary variable of the wanted type to store
> >> the header if the memory in the iovec array is not contiguous.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>   iov.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
> >>   iov.h     | 52 ++++++++++++++++++++++++++++++++++++++--------------
> >>   tcp_buf.c |  2 +-
> >>   3 files changed, 83 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/iov.c b/iov.c
> >> index 047fcbce7fcd..907cd5339369 100644
> >> --- a/iov.c
> >> +++ b/iov.c
> >> @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
> >>    *
> >>    * Returns:    The number of bytes successfully copied.
> >>    */
> >> -/* cppcheck-suppress unusedFunction */
> >> +/* cppcheck-suppress [staticFunction] */
> >>   size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
> >>   		  size_t offset, void *buf, size_t bytes)
> >>   {
> >> @@ -126,6 +126,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
> >>   	/* copying data */
> >>   	for (copied = 0; copied < bytes && i < iov_cnt; i++) {
> >>   		size_t len = MIN(iov[i].iov_len - offset, bytes - copied);
> >> +		/* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */
> >>   		memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset,
> >>   		       len);
> >>   		copied += len;
> >> @@ -260,7 +261,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len)
> >>   }
> >>   
> >>   /**
> >> - * iov_peek_header_() - Get pointer to a header from an IOV tail
> >> + * iov_check_header() - Check if a header can be accessed
> >>    * @tail:	IOV tail to get header from
> >>    * @len:	Length of header to get, in bytes
> >>    * @align:	Required alignment of header, in bytes
> >> @@ -271,8 +272,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len)
> >>    *	    overruns the IO vector, is not contiguous or doesn't have the
> >>    *	    requested alignment.
> >>    */
> >> -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
> >> -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
> >> +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t align)
> >>   {
> >>   	char *p;
> >>   
> >> @@ -292,27 +292,62 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
> >>   	return p;
> >>   }
> >>   
> >> +/**
> >> + * iov_peek_header_() - Get pointer to a header from an IOV tail
> >> + * @tail:	IOV tail to get header from
> >> + * @v:		Temporary memory to use if the memory in @tail
> >> + *		is discontinuous
> >> + * @len:	Length of header to get, in bytes
> >> + * @align:	Required alignment of header, in bytes
> >> + *
> >> + * @tail may be pruned, but will represent the same bytes as before.
> >> + *
> >> + * Returns: Pointer to the first @len logical bytes of the tail, or to
> >> + *          a copy if that overruns the IO vector, is not contiguous or
> >> + *          doesn't have the requested alignment. NULL if that overruns the
> >> + *          IO vector.
> >> + */
> >> +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
> >> +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
> >> +{
> >> +	char *p = iov_check_header(tail, len, align);
> >> +	size_t l;
> >> +
> >> +	if (p)
> >> +		return p;
> >> +
> >> +	l = iov_to_buf(tail->iov, tail->cnt,  tail->off, v, len);  
> > 
> > This effectively bypasses three checks performed by iov_check_header(),
> > that is, if there's nothing left in the iov_tail, if 'len' exceeds it,
> > or if it's not aligned, we'll proceed calling iov_to_buf(), whereas we
> > should only call it if the buffer is not contiguous, I think.
> > 
> > Perhaps it would make more sense to fail on iov_check_header() failure,
> > and take the contiguity check out of it, so that we preserve the early
> > return on those failures.
> > 
> > Another alternative might be to call iov_to_buf from the old version of
> > iov_peek_header_(), but I guess things would be easier to follow if
> > iov_check_header() really indicates failure, instead.  
> 
> I think it's correct to make a copy when it's not aligned (because it's related to the 
> data position in memory, like to be not contiguous).

Ah, hmm, right. Well, on one hand, it should never happen anyway, but
on the other hand, it's a mistake that could now become harmless with
your implementation, so why not...

> The only real error case we must manage is if len exceeds the remaining size in the 
> iov_tail: we check the return value iov_to_buf() for that.

There's also the if (!iov_tail_prune(tail)) case, but I guess the same
reasoning applies (I haven't checked in detail, though).

-- 
Stefano