Re: [PATCH v2 1/4] packet: replace struct desc by struct iovec

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6729 bytes --]

On Fri, Jul 12, 2024 at 05:32:41PM +0200, Laurent Vivier wrote:
> To be able to manage buffers inside a shared memory provided
> by a VM via a vhost-user interface, we cannot rely on the fact
> that buffers are located in a pre-defined memory area and use
> a base address and a 32bit offset to address them.
> 
> We need a 64bit address, so replace struct desc by struct iovec
> and update range checking.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c | 84 +++++++++++++++++++++++++++++++-------------------------
>  packet.h | 14 ++--------
>  2 files changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index ccfc84607709..f7bb523c4ffa 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -22,6 +22,39 @@
>  #include "util.h"
>  #include "log.h"
>  
> +/**
> + * packet_check_range() - Check if a packet memory range is valid
> + * @p:		Packet pool
> + * @offset:	Offset of data range in packet descriptor
> + * @len:	Length of desired data range
> + * @start:	Start of the packet descriptor
> + * @func:	For tracing: name of calling function, NULL means no trace()
> + * @line:	For tracing: caller line of function call
> + *
> + * Return: 0 if the range is valid, -1 otherwise
> + */
> +static int packet_check_range(const struct pool *p, size_t offset, size_t len,
> +			      const char *start, const char *func, int line)
> +{
> +	if (start < p->buf) {
> +		if (func) {

Omitting the message entirely if func is not set doesn't seem correct.
I believe printf() should format NULL pointers sanely (typically as
"<null>"), so I think you can just leave out this check.

> +			trace("add packet start %p before buffer start %p, "
> +			      "%s:%i", (void *)start, (void *)p->buf, func, line);
> +		}
> +		return -1;
> +	}
> +
> +	if (start + len + offset > p->buf + p->buf_size) {

It's not really clear to me why offset is needed in here.  AIUI,
offset is used when we want to talk about some piece of a larger
packet/frame that's in the buffer.  That's useful when we're
dissecting packets, but surely we always want the whole frame/whatever
to be within the buffer, so I don't know we need the extra complexity
in this helper.

I also think we should check for overflow on the LHS here, but that's
pre-existing, so it doesn't need to go in this patch.

> +		if (func) {
> +			trace("packet offset plus length %lu from size %lu, "
> +			      "%s:%i", start - p->buf + len + offset,
> +			      p->buf_size, func, line);
> +		}
> +		return -1;
> +	}
> +
> +	return 0;
> +}
>  /**
>   * packet_add_do() - Add data as packet descriptor to given pool
>   * @p:		Existing pool
> @@ -41,34 +74,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
>  		return;
>  	}
>  
> -	if (start < p->buf) {
> -		trace("add packet start %p before buffer start %p, %s:%i",
> -		      (void *)start, (void *)p->buf, func, line);
> +	if (packet_check_range(p, 0, len, start, func, line))
>  		return;
> -	}
> -
> -	if (start + len > p->buf + p->buf_size) {
> -		trace("add packet start %p, length: %zu, buffer end %p, %s:%i",
> -		      (void *)start, len, (void *)(p->buf + p->buf_size),
> -		      func, line);
> -		return;
> -	}
>  
>  	if (len > UINT16_MAX) {
>  		trace("add packet length %zu, %s:%i", len, func, line);
>  		return;
>  	}
>  
> -#if UINTPTR_MAX == UINT64_MAX
> -	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {
> -		trace("add packet start %p, buffer start %p, %s:%i",
> -		      (void *)start, (void *)p->buf, func, line);
> -		return;
> -	}
> -#endif
> -
> -	p->pkt[idx].offset = start - p->buf;
> -	p->pkt[idx].len = len;
> +	p->pkt[idx].iov_base = (void *)start;
> +	p->pkt[idx].iov_len = len;
>  
>  	p->count++;
>  }
> @@ -96,36 +111,31 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
>  		return NULL;
>  	}
>  
> -	if (len > UINT16_MAX || len + offset > UINT32_MAX) {
> +	if (len > UINT16_MAX) {
>  		if (func) {
> -			trace("packet data length %zu, offset %zu, %s:%i",
> -			      len, offset, func, line);
> +			trace("packet data length %zu, %s:%i",
> +			      len, func, line);

Should this be an assert?  Seems like something is wrong in the
caller, if they're trying to pass in a ludicrously long packet.

>  		}
>  		return NULL;
>  	}
>  
> -	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> +	if (len + offset > p->pkt[idx].iov_len) {
>  		if (func) {
> -			trace("packet offset plus length %zu from size %zu, "
> -			      "%s:%i", p->pkt[idx].offset + len + offset,
> -			      p->buf_size, func, line);
> +			trace("data length %zu, offset %zu from length %zu, "
> +			      "%s:%i", len, offset, p->pkt[idx].iov_len,
> +			      func, line);
>  		}
>  		return NULL;
>  	}
>  
> -	if (len + offset > p->pkt[idx].len) {
> -		if (func) {
> -			trace("data length %zu, offset %zu from length %u, "
> -			      "%s:%i", len, offset, p->pkt[idx].len,
> -			      func, line);
> -		}
> +	if (packet_check_range(p, offset, len, p->pkt[idx].iov_base,
> +			       func, line))
>  		return NULL;
> -	}
>  
>  	if (left)
> -		*left = p->pkt[idx].len - offset - len;
> +		*left = p->pkt[idx].iov_len - offset - len;
>  
> -	return p->buf + p->pkt[idx].offset + offset;
> +	return (char *)p->pkt[idx].iov_base + offset;
>  }
>  
>  /**
> diff --git a/packet.h b/packet.h
> index a784b07bbed5..8377dcf678bb 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -6,16 +6,6 @@
>  #ifndef PACKET_H
>  #define PACKET_H
>  
> -/**
> - * struct desc - Generic offset-based descriptor within buffer
> - * @offset:	Offset of descriptor relative to buffer start, 32-bit limit
> - * @len:	Length of descriptor, host order, 16-bit limit
> - */
> -struct desc {
> -	uint32_t offset;
> -	uint16_t len;
> -};
> -
>  /**
>   * struct pool - Generic pool of packets stored in a buffer
>   * @buf:	Buffer storing packet descriptors
> @@ -29,7 +19,7 @@ struct pool {
>  	size_t buf_size;
>  	size_t size;
>  	size_t count;
> -	struct desc pkt[1];
> +	struct iovec pkt[1];
>  };
>  
>  void packet_add_do(struct pool *p, size_t len, const char *start,
> @@ -54,7 +44,7 @@ struct _name ## _t {							\
>  	size_t buf_size;						\
>  	size_t size;							\
>  	size_t count;							\
> -	struct desc pkt[_size];						\
> +	struct iovec pkt[_size];					\
>  }
>  
>  #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size)			\

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]