On 11/2/26 08:30, luochunsheng@ustc.edu wrote:
From: luffyluo
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
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);