On Tue, Aug 05, 2025 at 05:46:23PM +0200, Laurent Vivier wrote:
As the iov_tail has a non zero offset (because of the presence of packet headers in the iov array), we must copy it to a new iov array (using iov_tail_splice()) to pass it to sendmsg().
We cannot use anymore iov_tail_msghdr(), so remove it.
Right, as I mentioned on that patch, it was always kind of a foot gun. Was this the only user? Maybe just open code it earlier in the series rather than introducing then removing the helper.
Signed-off-by: Laurent Vivier
Reviewed-by: David Gibson
--- icmp.c | 30 +++++++++++++++++++----------- icmp.h | 2 +- iov.c | 23 ----------------------- iov.h | 2 -- tap.c | 7 ++----- 5 files changed, 22 insertions(+), 42 deletions(-)
diff --git a/icmp.c b/icmp.c index 71c496540310..be800e30c369 100644 --- a/icmp.c +++ b/icmp.c @@ -44,6 +44,7 @@
#define ICMP_ECHO_TIMEOUT 60 /* s, timeout for ICMP socket activity */ #define ICMP_NUM_IDS (1U << 16) +#define MAX_IOV_ICMP 16 /* Arbitrary, should be enough */
/** * ping_at_sidx() - Get ping specific flow at given sidx @@ -229,36 +230,33 @@ cancel: * @af: Address family, AF_INET or AF_INET6 * @saddr: Source address * @daddr: Destination address - * @p: Packet pool, single packet with ICMP/ICMPv6 header + * @data: Single packet with ICMP/ICMPv6 header * @now: Current timestamp * * Return: count of consumed packets (always 1, even if malformed) */ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, - const struct pool *p, const struct timespec *now) + struct iov_tail *data, const struct timespec *now) { + struct iovec iov[MAX_IOV_ICMP]; struct icmp_ping_flow *pingf; const struct flowside *tgt; union sockaddr_inany sa; - struct iov_tail data; struct msghdr msh; uint16_t id, seq; union flow *flow; uint8_t proto; - socklen_t sl; + int cnt;
(void)saddr; ASSERT(pif == PIF_TAP);
- if (!packet_get(p, 0, &data)) - return -1; - if (af == AF_INET) { struct icmphdr ih_storage; const struct icmphdr *ih;
- ih = IOV_PEEK_HEADER(&data, ih_storage); + ih = IOV_PEEK_HEADER(data, ih_storage); if (!ih) return 1;
@@ -272,7 +270,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, struct icmp6hdr ih_storage; const struct icmp6hdr *ih;
- ih = IOV_PEEK_HEADER(&data, ih_storage); + ih = IOV_PEEK_HEADER(data, ih_storage); if (!ih) return 1;
@@ -286,6 +284,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, ASSERT(0); }
+ cnt = iov_tail_clone(&iov[0], MAX_IOV_ICMP, data); + if (cnt < 0) + return 1; + flow = flow_at_sidx(flow_lookup_af(c, proto, PIF_TAP, af, saddr, daddr, id, id));
@@ -300,8 +302,14 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, pingf->ts = now->tv_sec;
- pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0); - iov_tail_msghdr(&msh, &data, &sa, sl); + pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0); + msh.msg_name = &sa; + msh.msg_iov = iov; + msh.msg_iovlen = cnt; + msh.msg_control = NULL; + msh.msg_controllen = 0; + msh.msg_flags = 0; + if (sendmsg(pingf->sock, &msh, MSG_NOSIGNAL) < 0) { flow_dbg_perror(pingf, "failed to relay request to socket"); } else { diff --git a/icmp.h b/icmp.h index 5ce22b5eca1f..d1cecb20e29d 100644 --- a/icmp.h +++ b/icmp.h @@ -14,7 +14,7 @@ struct icmp_ping_flow; void icmp_sock_handler(const struct ctx *c, union epoll_ref ref); int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, const void *saddr, const void *daddr, - const struct pool *p, const struct timespec *now); + struct iov_tail *data, const struct timespec *now); void icmp_init(void);
/** diff --git a/iov.c b/iov.c index d17d4dd3da09..1d734acdfea6 100644 --- a/iov.c +++ b/iov.c @@ -157,29 +157,6 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) return len; }
-/** - * iov_tail_msghdr - Initialize a msghdr from an IOV tail structure - * @msh: msghdr to initialize - * @tail: iov_tail to use to set msg_iov and msg_iovlen - * @msg_name: Pointer to set to msg_name - * @msg_namelen: Size of @msg_name - */ -void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail, - void *msg_name, socklen_t msg_namelen) -{ - iov_tail_prune(tail); - - ASSERT(tail->off == 0); - - msh->msg_name = msg_name; - msh->msg_namelen = msg_namelen; - msh->msg_iov = (struct iovec *)tail->iov; - msh->msg_iovlen = tail->cnt; - msh->msg_control = NULL; - msh->msg_controllen = 0; - msh->msg_flags = 0; -} - /** * iov_tail_prune() - Remove any unneeded buffers from an IOV tail * @tail: IO vector tail (modified) diff --git a/iov.h b/iov.h index 75c3b07a87e3..ccdb690ef3f1 100644 --- a/iov.h +++ b/iov.h @@ -82,8 +82,6 @@ struct iov_tail { 1, \ (off_))
-void iov_tail_msghdr(struct msghdr *msh, struct iov_tail *tail, - void *msg_name, socklen_t msg_namelen); bool iov_tail_prune(struct iov_tail *tail); size_t iov_tail_size(struct iov_tail *tail); bool iov_tail_drop(struct iov_tail *tail, size_t len); diff --git a/tap.c b/tap.c index 3262b44c4287..48152a84674c 100644 --- a/tap.c +++ b/tap.c @@ -764,17 +764,14 @@ resume: continue;
if (iph->protocol == IPPROTO_ICMP) { - PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); - if (c->no_icmp) continue;
tap_packet_debug(iph, NULL, NULL, 0, NULL, 1);
- packet_add(pkt, &data); icmp_tap_handler(c, PIF_TAP, AF_INET, &iph->saddr, &iph->daddr, - pkt, now); + &data, now); continue; }
@@ -964,7 +961,7 @@ resume: tap_packet_debug(NULL, ip6h, NULL, proto, NULL, 1);
icmp_tap_handler(c, PIF_TAP, AF_INET6, - saddr, daddr, pkt, now); + saddr, daddr, &data, now); continue; }
-- 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