[PATCH] packet: Fix pool traversal for multi-iovec packets

[PATCH] packet: Fix pool traversal for multi-iovec packets

[luochunsheng]

From: luffyluo <luffyluo@tencent.com>

With vhost-user a guest frame can span several descriptors.  The pool
stores such a packet as a marker slot (iov_base == NULL, iov_len = N)
followed by N data slots, and packet_get() uses the marker to return
all N iovecs at once.

The traversal loops in tap4_handler(), tap6_handler(),
tcp_data_from_tap() and udp_tap_handler() don't know about markers and
advance one slot at a time: after handling the packet from its marker
they land on its first data slot, where iov_base != NULL, so
packet_get() hands back that slot alone, as if it were a whole
single-iovec packet.  Parsing that fragment as a complete frame is
what breaks: in tcp_data_from_tap() a fragment shorter than a TCP
header makes IOV_PEEK_HEADER() fail ("TCP header not contiguous"), and
a longer one gives a th->doff read from payload bytes, so
off = th->doff * 4 exceeds the fragment length.  In udp_tap_handler()
the fragments go out as extra datagrams.

Fix it with packet_next(), returning the index of the packet following
the one at a given index, and use it to advance those loops, so they
skip the data slots of the packet they just handled.  For single-iovec
packets it returns idx + 1, so tap and pasta, which always build
single-iovec tails with IOV_TAIL_FROM_BUF(), are unaffected.

Fixes: ca38be0141d1 ("packet: Add support for multi-vector packets")
Signed-off-by: luffyluo <luffyluo@tencent.com>
---
 packet.c | 16 ++++++++++++++++
 packet.h |  1 +
 tap.c    |  4 ++--
 tcp.c    |  2 +-
 udp.c    |  7 +++++--
 5 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/packet.c b/packet.c
index 7a347be..f202245 100644
--- a/packet.c
+++ b/packet.c
@@ -207,3 +207,19 @@ void pool_flush(struct pool *p)
 {
 	p->count = 0;
 }
+
+/**
+ * packet_next() - Index of the packet following the one at @idx
+ * @p:		Packet pool
+ * @idx:	Slot index of the current packet (marker or single iovec)
+ *
+ * A multi-iovec packet is stored as a marker slot (iov_base == NULL) whose
+ * iov_len holds the number of iovec slots that follow it. Return the slot
+ * index of the next packet.
+ */
+size_t packet_next(const struct pool *p, size_t idx)
+{
+	if (p->pkt[idx].iov_base == NULL)
+		return idx + p->pkt[idx].iov_len + 1;
+	return idx + 1;
+}
diff --git a/packet.h b/packet.h
index 7b7fa71..03821f6 100644
--- a/packet.h
+++ b/packet.h
@@ -41,6 +41,7 @@ bool packet_get_do(const struct pool *p, const size_t idx,
 		   struct iov_tail *data, const char *func, int line);
 bool pool_can_fit(const struct pool *p, struct iov_tail *data);
 void pool_flush(struct pool *p);
+size_t packet_next(const struct pool *p, size_t idx);
 
 #define packet_add(p, data)					\
 	packet_add_do(p, data, __func__, __LINE__)
diff --git a/tap.c b/tap.c
index dfa66c7..1a310fe 100644
--- a/tap.c
+++ b/tap.c
@@ -700,7 +700,7 @@ static int tap4_handler(struct ctx *c, const struct pool *in,
 
 	i = 0;
 resume:
-	for (seq_count = 0, seq = NULL; i < in->count; i++) {
+	for (seq_count = 0, seq = NULL; i < in->count; i = packet_next(in, i)) {
 		struct iovec trim_iov[UIO_MAXIOV];
 		size_t l3len, hlen, l4len, check;
 		struct ethhdr eh_storage;
@@ -948,7 +948,7 @@ static int tap6_handler(struct ctx *c, const struct pool *in,
 
 	i = 0;
 resume:
-	for (seq_count = 0, seq = NULL; i < in->count; i++) {
+	for (seq_count = 0, seq = NULL; i < in->count; i = packet_next(in, i)) {
 		size_t l4len, plen, check;
 		struct in6_addr *saddr, *daddr;
 		struct ipv6hdr ip6h_storage;
diff --git a/tcp.c b/tcp.c
index 95f4f3e..a3bb8b9 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1946,7 +1946,7 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
 
 	assert(conn->events & ESTABLISHED);
 
-	for (i = idx, iov_i = 0; i < (int)p->count; i++) {
+	for (i = idx, iov_i = 0; i < (int)p->count; i = packet_next(p, i)) {
 		uint32_t seq, seq_offset, ack_seq;
 		struct tcphdr th_storage;
 		const struct tcphdr *th;
diff --git a/udp.c b/udp.c
index 505e554..bd5e241 100644
--- a/udp.c
+++ b/udp.c
@@ -1029,6 +1029,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 	const struct udphdr *uh;
 	struct udp_flow *uflow;
 	int i, j, s, count = 0;
+	size_t slot;
 	struct iov_tail data;
 	flow_sidx_t tosidx;
 	in_port_t src, dst;
@@ -1077,10 +1078,12 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 
 	pif_sockaddr(c, &to_sa, topif, &toside->eaddr, toside->eport);
 
-	for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) {
+	for (i = 0, j = 0, slot = idx;
+	     slot < p->count && j < UIO_MAXIOV;
+	     slot = packet_next(p, slot), i++) {
 		const struct udphdr *uh_send;
 
-		if (!packet_get(p, idx + i, &data))
+		if (!packet_get(p, slot, &data))
 			return p->count - idx;
 
 		uh_send = IOV_REMOVE_HEADER(&data, uh_storage);
-- 
2.43.7

Re: [PATCH] packet: Fix pool traversal for multi-iovec packets

[Laurent Vivier]

On 11/2/26 08:30, luochunsheng@ustc.edu wrote:
> From: luffyluo <luffyluo@tencent.com>
> 
> With vhost-user a guest frame can span several descriptors.  The pool
> stores such a packet as a marker slot (iov_base == NULL, iov_len = N)
> followed by N data slots, and packet_get() uses the marker to return
> all N iovecs at once.
> 
> The traversal loops in tap4_handler(), tap6_handler(),
> tcp_data_from_tap() and udp_tap_handler() don't know about markers and
> advance one slot at a time: after handling the packet from its marker
> they land on its first data slot, where iov_base != NULL, so
> packet_get() hands back that slot alone, as if it were a whole
> single-iovec packet.  Parsing that fragment as a complete frame is
> what breaks: in tcp_data_from_tap() a fragment shorter than a TCP
> header makes IOV_PEEK_HEADER() fail ("TCP header not contiguous"), and
> a longer one gives a th->doff read from payload bytes, so
> off = th->doff * 4 exceeds the fragment length.  In udp_tap_handler()
> the fragments go out as extra datagrams.
> 
> Fix it with packet_next(), returning the index of the packet following
> the one at a given index, and use it to advance those loops, so they
> skip the data slots of the packet they just handled.  For single-iovec
> packets it returns idx + 1, so tap and pasta, which always build
> single-iovec tails with IOV_TAIL_FROM_BUF(), are unaffected.
> 
> Fixes: ca38be0141d1 ("packet: Add support for multi-vector packets")
> Signed-off-by: luffyluo <luffyluo@tencent.com>

Thank you for your patch, it looks correct.

There is a remaining problem:

the handlers return the number of consumed slots (generally they comsume everything so 
they return p->count - idx) but udp_tap_handler() returns the return value of sendmmsg() 
that doesn't reflect the actual number of consumed slot.

Thanks,
Laurent
> ---
>   packet.c | 16 ++++++++++++++++
>   packet.h |  1 +
>   tap.c    |  4 ++--
>   tcp.c    |  2 +-
>   udp.c    |  7 +++++--
>   5 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index 7a347be..f202245 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -207,3 +207,19 @@ void pool_flush(struct pool *p)
>   {
>   	p->count = 0;
>   }
> +
> +/**
> + * packet_next() - Index of the packet following the one at @idx
> + * @p:		Packet pool
> + * @idx:	Slot index of the current packet (marker or single iovec)
> + *
> + * A multi-iovec packet is stored as a marker slot (iov_base == NULL) whose
> + * iov_len holds the number of iovec slots that follow it. Return the slot
> + * index of the next packet.
> + */
> +size_t packet_next(const struct pool *p, size_t idx)
> +{
> +	if (p->pkt[idx].iov_base == NULL)
> +		return idx + p->pkt[idx].iov_len + 1;
> +	return idx + 1;
> +}
> diff --git a/packet.h b/packet.h
> index 7b7fa71..03821f6 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -41,6 +41,7 @@ bool packet_get_do(const struct pool *p, const size_t idx,
>   		   struct iov_tail *data, const char *func, int line);
>   bool pool_can_fit(const struct pool *p, struct iov_tail *data);
>   void pool_flush(struct pool *p);
> +size_t packet_next(const struct pool *p, size_t idx);
>   
>   #define packet_add(p, data)					\
>   	packet_add_do(p, data, __func__, __LINE__)
> diff --git a/tap.c b/tap.c
> index dfa66c7..1a310fe 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -700,7 +700,7 @@ static int tap4_handler(struct ctx *c, const struct pool *in,
>   
>   	i = 0;
>   resume:
> -	for (seq_count = 0, seq = NULL; i < in->count; i++) {
> +	for (seq_count = 0, seq = NULL; i < in->count; i = packet_next(in, i)) {
>   		struct iovec trim_iov[UIO_MAXIOV];
>   		size_t l3len, hlen, l4len, check;
>   		struct ethhdr eh_storage;
> @@ -948,7 +948,7 @@ static int tap6_handler(struct ctx *c, const struct pool *in,
>   
>   	i = 0;
>   resume:
> -	for (seq_count = 0, seq = NULL; i < in->count; i++) {
> +	for (seq_count = 0, seq = NULL; i < in->count; i = packet_next(in, i)) {
>   		size_t l4len, plen, check;
>   		struct in6_addr *saddr, *daddr;
>   		struct ipv6hdr ip6h_storage;
> diff --git a/tcp.c b/tcp.c
> index 95f4f3e..a3bb8b9 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1946,7 +1946,7 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>   
>   	assert(conn->events & ESTABLISHED);
>   
> -	for (i = idx, iov_i = 0; i < (int)p->count; i++) {
> +	for (i = idx, iov_i = 0; i < (int)p->count; i = packet_next(p, i)) {
>   		uint32_t seq, seq_offset, ack_seq;
>   		struct tcphdr th_storage;
>   		const struct tcphdr *th;
> diff --git a/udp.c b/udp.c
> index 505e554..bd5e241 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1029,6 +1029,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>   	const struct udphdr *uh;
>   	struct udp_flow *uflow;
>   	int i, j, s, count = 0;
> +	size_t slot;
>   	struct iov_tail data;
>   	flow_sidx_t tosidx;
>   	in_port_t src, dst;
> @@ -1077,10 +1078,12 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>   
>   	pif_sockaddr(c, &to_sa, topif, &toside->eaddr, toside->eport);
>   
> -	for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) {
> +	for (i = 0, j = 0, slot = idx;
> +	     slot < p->count && j < UIO_MAXIOV;
> +	     slot = packet_next(p, slot), i++) {
>   		const struct udphdr *uh_send;
>   
> -		if (!packet_get(p, idx + i, &data))
> +		if (!packet_get(p, slot, &data))
>   			return p->count - idx;
>   
>   		uh_send = IOV_REMOVE_HEADER(&data, uh_storage);