Re: [PATCH 16/24] packet: replace struct desc by struct iovec

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 6 Feb 2024 13:25:18 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Feb 02, 2024 at 03:11:43PM +0100, Laurent Vivier wrote:
> 
> Rationale please.  It's probably also worth nothing that this does
> replace struct desc with a larger structure - struct desc is already
> padded out to 8 bytes, but on 64-bit machines iovec will be larger
> still.

Right, yes, that becomes 16 bytes (from 8), and those arrays are
already quite large. I wonder if we can keep struct desc, but I have no
idea how complicated it is.

> > Signed-off-by: Laurent Vivier <lvivier@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
> > @@ -22,6 +22,36 @@
> >  #include "util.h"
> >  #include "log.h"
> >  
> > +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) {
> > +			trace("add packet start %p before buffer start %p, "
> > +			      "%s:%i", (void *)start, (void *)p->buf, func, line);
> > +		}
> > +		return -1;  
> 
> I guess not really in scope for this patch, but IIUC the only place
> we'd hit this is if the caller has done something badly wrong, so
> possibly it should be an ASSERT().

I wouldn't terminate passt in any of these cases, because that can turn
a safety check into a possibility for a denial of service. These checks
are here not to avoid obvious logic issues, but rather to make them
less harmful if present.

In general, we assume that the hypervisor is able to wreck its own
connectivity, but we should make it harder for users in the guest to do
so.

> > +	}
> > +
> > +	if (start + len + offset > p->buf + p->buf_size) {
> > +		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;
> > +	}  
> 
> Same with this one.
> 
> > +
> > +#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 -1;
> > +	}
> > +#endif  
> 
> This one is relevant to this patch though - because you're expanding
> struct desc's 32-bit offset to full void * from struct iovec, this
> check is no longer relevant.
> 
> > +
> > +	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);
> >  		}
> >  		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)			\  
> 

-- 
Stefano