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

On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent Vivier wrote: Needs a commit message.

Signed-off-by: Laurent Vivier --- 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"

Function comment, please.

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

Pre-existing, but I wonder if these should be assert()s. Are there any cases where we'd hit this path that don't indicate a bug in the caller?

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

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

+ 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 :/

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

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