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

On Thu, Jul 04, 2024 at 05:52:09PM +0200, Laurent Vivier wrote:
...
  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: 
Well, I think we should be more careful here, but as you say I don't
think it necessarily belongs as part of this series.

...
 
 
  +		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. 
Hmm, yeah, I guess so.

...
 
  
    		}
   		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.

Ok, makes sense.

...
 
    * 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. 
Hm, ok.

-- 
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