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

4 Jul 2024

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

    