From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202508 header.b=mN/Ym+6D; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6C15C5A028A for ; Wed, 06 Aug 2025 08:35:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1754462131; bh=VN9ZiK9YCilOU7S99jx1GIGioAOENfnV98f/XFhE6rw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mN/Ym+6DzlnLxHjjpCmWNBxetU3cBgNWKveOuNrTFivwO0yiWR3hDLcV9SqCacmsD HQ7nnGBsK9XqrhhoQh9hF6TY6kDTanaXP4H4RizaFJgVC9BzQIrncOpWeRJbhruUmc vQiXavSmjHpmtUCZsHq7Z3Vty6KOdvUfpNm0+QOitikGeb8cNfAJgLiqlTosQIImtk cYbctadxsZTGAWMat22yt/NpXU2mWmqrXm/0k/ZA5493Y3bpebbXkMFdZW0FlKHqhr T5sbBh5jnmL/HDlL5kVbebZHFqR8WsaqAsuCPihm+L4iB+mCtB6sHKw8pATqioZoeh 05B/wDjaXq0jA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4bxgXM1SyPz4xcs; Wed, 6 Aug 2025 16:35:31 +1000 (AEST) Date: Wed, 6 Aug 2025 16:29:46 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v8 25/30] icmp: use iov_tail rather than pool Message-ID: References: <20250805154628.301343-1-lvivier@redhat.com> <20250805154628.301343-26-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="+IdWZEZ4OY/GRU+Q" Content-Disposition: inline In-Reply-To: <20250805154628.301343-26-lvivier@redhat.com> Message-ID-Hash: 4TQ4F3DZLFM7UE4FTVQ67PLQN7TSJAZI X-Message-ID-Hash: 4TQ4F3DZLFM7UE4FTVQ67PLQN7TSJAZI X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --+IdWZEZ4OY/GRU+Q Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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(). >=20 > 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. >=20 > 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(-) >=20 > diff --git a/icmp.c b/icmp.c > index 71c496540310..be800e30c369 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -44,6 +44,7 @@ > =20 > #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 */ > =20 > /** > * 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; > =20 > (void)saddr; > ASSERT(pif =3D=3D PIF_TAP); > =20 > - if (!packet_get(p, 0, &data)) > - return -1; > - > if (af =3D=3D AF_INET) { > struct icmphdr ih_storage; > const struct icmphdr *ih; > =20 > - ih =3D IOV_PEEK_HEADER(&data, ih_storage); > + ih =3D IOV_PEEK_HEADER(data, ih_storage); > if (!ih) > return 1; > =20 > @@ -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; > =20 > - ih =3D IOV_PEEK_HEADER(&data, ih_storage); > + ih =3D IOV_PEEK_HEADER(data, ih_storage); > if (!ih) > return 1; > =20 > @@ -286,6 +284,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pi= f, sa_family_t af, > ASSERT(0); > } > =20 > + cnt =3D iov_tail_clone(&iov[0], MAX_IOV_ICMP, data); > + if (cnt < 0) > + return 1; > + > flow =3D flow_at_sidx(flow_lookup_af(c, proto, PIF_TAP, > af, saddr, daddr, id, id)); > =20 > @@ -300,8 +302,14 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pi= f, sa_family_t af, > pingf->ts =3D now->tv_sec; > =20 > =20 > - 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 =3D &sa; > + msh.msg_iov =3D iov; > + msh.msg_iovlen =3D cnt; > + msh.msg_control =3D NULL; > + msh.msg_controllen =3D 0; > + msh.msg_flags =3D 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); > =20 > /** > 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; > } > =20 > -/** > - * 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 =3D=3D 0); > - > - msh->msg_name =3D msg_name; > - msh->msg_namelen =3D msg_namelen; > - msh->msg_iov =3D (struct iovec *)tail->iov; > - msh->msg_iovlen =3D tail->cnt; > - msh->msg_control =3D NULL; > - msh->msg_controllen =3D 0; > - msh->msg_flags =3D 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_)) > =20 > -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; > =20 > if (iph->protocol =3D=3D IPPROTO_ICMP) { > - PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > - > if (c->no_icmp) > continue; > =20 > tap_packet_debug(iph, NULL, NULL, 0, NULL, 1); > =20 > - packet_add(pkt, &data); > icmp_tap_handler(c, PIF_TAP, AF_INET, > &iph->saddr, &iph->daddr, > - pkt, now); > + &data, now); > continue; > } > =20 > @@ -964,7 +961,7 @@ resume: > tap_packet_debug(NULL, ip6h, NULL, proto, NULL, 1); > =20 > icmp_tap_handler(c, PIF_TAP, AF_INET6, > - saddr, daddr, pkt, now); > + saddr, daddr, &data, now); > continue; > } > =20 --=20 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 --+IdWZEZ4OY/GRU+Q Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmiS9k0ACgkQzQJF27ox 2GcSrxAAj65HaVhVU2Msdcf0RwksOCZBhdmbOOS+qG/c5yDNHK7XnEFedDjsw6uC njJVOmcnQCBt6dZ2m1yoAj8QFcHGVC+ZniTVCGGHXM+epYUE2zLYqiiiA/5Fo1zY ogSWxi9NKOVMNSpyTpQH6WiQ8Mpmyd0PiA2O7hHHF+y52DbjPpByEQ2ax0Wq/d6k 8V7izSh7no2T78HnLUVq0iNVJUaPii6AUNLf1XMmwqSmKKU1wCXJ1/sjQ3NlC17m FRsD8MpOE7L4Rz62dPgZxZ3Zog/P5iwAKFV0dGdTW0NK2KGVOikLtz2JeW8GnlgJ n3Lyb+rIjE8Zg/ITtvAKFj5vOpdZaOtQXtT/zjdiqVZHrbmuFDh0ITs6WHoRJUe0 MUKqNaLROfTyre5JBlaS9cn7Us722Y/G4xdZyayE1ivGBHyo3K7/q8RtxYGddvcw aDrq8ioC0F5/D0Vi2/l51TvmfbAlv/HuJ2xub6yIVCgRXAFvpxmfqgxwU/ViBbH5 vWMsV67Vigpm13JgLvceDPY3ZU9EfjcqvPwAAql19zrEph2ZrcOdK2F/OeHG8pAW VFSCItnHu7JwAA3T/J2ZGhhB75B8Px18HNOZlQdzjI+lCxZRpNa13Hc3cLMTnrQb jNMK4nkkxJe+yorPDCH6NN/4loj/Ao2ZwRJJMU53lGBmxVoe3Zw= =Kvay -----END PGP SIGNATURE----- --+IdWZEZ4OY/GRU+Q--