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

On 24/06/2024 04:48, David Gibson wrote:
...
  On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent
Vivier wrote:
 
 Needs a commit message.
 
> Signed-off-by: Laurent Vivier <lvivier(a)redhat.com>
> ---
>   packet.c | 75 +++++++++++++++++++++++++++++++-------------------------
>   packet.h | 14 ++---------
>   2 files changed, 43 insertions(+), 46 deletions(-)
>
> diff --git a/packet.c b/packet.c
> index ccfc84607709..af2a539a1794 100644
> --- a/packet.c
> +++ b/packet.c 
...
...
 
  +	}
 +
 +	if (start + len + offset > p->buf + p->buf_size) { 
 
 Also pre-existing, but I wonder if we should check for overflow of
 (Start + len + offset). 
Originally, I didn't want to change the existing behaviour. Only to move code, and to
use 
a common function for packet_add_do() and packet_get_do().
But if you think it should be better I can update the code for that:

...
 
  +		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;
 +	}
 +
 +#if UINTPTR_MAX == UINT64_MAX
 +	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { 
 
 I don't think this check is relevant any more if we're going to iovecs
 - this was just because the offset in struct desc was only 32-bit. 
I agree.

...
  
  +		trace("add packet start %p, buffer start
%p, %s:%i",
 +		      (void *)start, (void *)p->buf, func, line);
 +		return -1;
 +	}
 +#endif
 +
 +	return 0;
 +}
   /**
    * packet_add_do() - Add data as packet descriptor to given pool
    * @p:		Existing pool
 @@ -41,34 +71,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++;
   }
 @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t
offset,
   		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); 
 
 I'm not sure either the old or new message is particularly descriptive
 here :/ 
I think the func and line parameters will help to understand the problem, and the others 
why the trace is triggered.

...
  
    		}
   		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)) 
 
 Ah.. right..  in this case we certainly don't want ASSERT()s in
 packet_check_range().  Still wonder if that would make more sense for
 the packet add case, however.
 
 A couple of other points:
   * You've effectively switched the order of the two different tests here
     (one range checking against the entire buffer, one range checking
     against a single packet).  Any reason for that? 
The idea is to check the parameters are valid before checking the buffer is valid.

...
    * Do we actually need the entire-buffer check here
on the _get()
     side?  Isn't it enough to ensure that packets lie within the buffer
     when they're inserted?  Pre-existing, again, AFAICT. 
I wanted to keep the idea introduced in bb708111833e ("treewide: Packet abstraction
with 
mandatory boundary checks") and checking we don't read outside of the buffer.

Thanks,
Laurent