From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 9BA605A0274 for ; Sun, 7 Jan 2024 05:28:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1704601731; bh=482q26dnZ/EPKfliKS91nzvRDVr/tYM9PruFS+QVXTU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CBii4iVruONJe2Buuy7KLpoLy922rSOLuNDhSkgptijT62Wy0gkeJCv4yeQLm5pml tntaOneEwj+zZH7WTKGWyZhLkzNiKl3Lr/Oj9+fRSnZrHKGrIcrFRUuOHesIVNhiIt o3K4VvCRD7miTUnV4n63e0lEWHid2JuLWlOcoMECcs536Pc17VvSNwsoAe5VF5Jht3 Zp9ICd44GOR3COserQMFA57r51t1RPJn08LZuCJ47vs8MjR9vTchVcN4DjTKfAoesm cl3Uv16zvuGQteYwMxw+P/cKSeGFvTWGV9fQEbEqyagvgxG4JRl8HXplGJvdehsuLu QAB1ZzeqJtPhg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4T742W5s7kz4wny; Sun, 7 Jan 2024 15:28:51 +1100 (AEDT) Date: Sun, 7 Jan 2024 11:45:02 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 09/12] icmp: Consolidate icmp_sock_handler() with icmpv6_sock_handler() Message-ID: References: <20231221065327.1307827-1-david@gibson.dropbear.id.au> <20231221065327.1307827-10-david@gibson.dropbear.id.au> <20240106170044.0aa57f62@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rRUgzLQ9bkcy/8hZ" Content-Disposition: inline In-Reply-To: <20240106170044.0aa57f62@elisabeth> Message-ID-Hash: KNZPVYIF2YCSWDCDFYX2T7JUHWR6RB3P X-Message-ID-Hash: KNZPVYIF2YCSWDCDFYX2T7JUHWR6RB3P 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: --rRUgzLQ9bkcy/8hZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 06, 2024 at 05:00:44PM +0100, Stefano Brivio wrote: > On Thu, 21 Dec 2023 17:53:24 +1100 > David Gibson wrote: >=20 > > Currently we have separate handlers for ICMP and ICMPv6 ping replies. > > Although there are a number of points of difference, with some creative > > refactoring we can combine these together sensibly. Although it doesn't > > save a vast amount of code, it does make it clearer that we're performi= ng > > basically the same steps for each case. > >=20 > > Signed-off-by: David Gibson > > --- > > icmp.c | 90 ++++++++++++++++++++++----------------------------------- > > icmp.h | 3 +- > > passt.c | 4 +-- > > 3 files changed, 37 insertions(+), 60 deletions(-) > >=20 > > diff --git a/icmp.c b/icmp.c > > index f6c744a..b26c61f 100644 > > --- a/icmp.c > > +++ b/icmp.c > > @@ -57,85 +57,63 @@ struct icmp_id_sock { > > static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; > > =20 > > /** > > - * icmp_sock_handler() - Handle new data from IPv4 ICMP socket > > + * icmp_sock_handler() - Handle new data from ICMP or ICMPv6 socket > > * @c: Execution context > > + * @af: Address family (AF_INET or AF_INET6) > > * @ref: epoll reference > > */ > > -void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > +void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref re= f) > > { > > + struct icmp_id_sock *const id_map =3D af =3D=3D AF_INET > > + ? &icmp_id_map[V4][ref.icmp.id] : &icmp_id_map[V6][ref.icmp.id]; > > + const char *const pname =3D af =3D=3D AF_INET ? "ICMP" : "ICMPv6"; > > char buf[USHRT_MAX]; > > - struct icmphdr *ih =3D (struct icmphdr *)buf; > > - struct sockaddr_in sr; > > + union { > > + struct sockaddr sa; > > + struct sockaddr_in sa4; > > + struct sockaddr_in6 sa6; > > + } sr; > > socklen_t sl =3D sizeof(sr); > > - uint16_t seq, id; > > + uint16_t seq; > > ssize_t n; > > =20 > > if (c->no_icmp) > > return; > > =20 > > - n =3D recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &= sl); > > + n =3D recvfrom(ref.fd, buf, sizeof(buf), 0, &sr.sa, &sl); > > if (n < 0) > > return; > > =20 > > - seq =3D ntohs(ih->un.echo.sequence); > > - > > - /* Adjust the packet to have the ID the guest was using, rather than = the > > - * host chosen value */ > > - id =3D ref.icmp.id; > > - ih->un.echo.id =3D htons(id); > > + if (af =3D=3D AF_INET) { > > + struct icmphdr *ih4 =3D (struct icmphdr *)buf; > > =20 > > - if (c->mode =3D=3D MODE_PASTA) { > > - if (icmp_id_map[V4][id].seq =3D=3D seq) > > - return; > > + /* Adjust packet back to guest-side ID */ > > + ih4->un.echo.id =3D htons(ref.icmp.id); > > + seq =3D ntohs(ih4->un.echo.sequence); > > + } else if (af =3D=3D AF_INET6) { > > + struct icmp6hdr *ih6 =3D (struct icmp6hdr *)buf; > > =20 > > - icmp_id_map[V4][id].seq =3D seq; > > + /* Adjust packet back to guest-side ID */ > > + ih6->icmp6_identifier =3D htons(ref.icmp.id); > > + seq =3D ntohs(ih6->icmp6_sequence); > > + } else { > > + ASSERT(0); > > } > > =20 > > - debug("ICMP: echo reply to tap, ID: %i, seq: %i", id, seq); > > - > > - tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n); > > -} > > - > > -/** > > - * icmpv6_sock_handler() - Handle new data from ICMPv6 socket > > - * @c: Execution context > > - * @ref: epoll reference > > - */ > > -void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref) > > -{ > > - char buf[USHRT_MAX]; > > - struct icmp6hdr *ih =3D (struct icmp6hdr *)buf; > > - struct sockaddr_in6 sr; > > - socklen_t sl =3D sizeof(sr); > > - uint16_t seq, id; > > - ssize_t n; > > - > > - if (c->no_icmp) > > - return; > > - > > - n =3D recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &= sl); > > - if (n < 0) > > - return; > > - > > - seq =3D ntohs(ih->icmp6_sequence); > > - > > - /* Adjust the packet to have the ID the guest was using, rather than = the > > - * host chosen value */ > > - id =3D ref.icmp.id; > > - ih->icmp6_identifier =3D htons(id); > > - > > - /* In PASTA mode, we'll get any reply we send, discard them. */ > > if (c->mode =3D=3D MODE_PASTA) { >=20 > The comment here should be kept -- the kernel behaviour was rather > surprising to me, and I would have otherwise no idea why we return > early here. Good point, comment restored. --=20 David Gibson | 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 --rRUgzLQ9bkcy/8hZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWZ9A0ACgkQzQJF27ox 2Gcg2g/+ImIBtDdpvwmfGnNlNcoiKwv1W8cSFQOQOuoMkbLZDTFlQhMVXK+58XjG jg+tmY1LqZKdk4iqjvhvWToxwrfI7tH00YFzSm8Kd3hlUCgU/WHdEd3Q2u4NgOQ9 hw3qz5oInLZFABg5f1+yUKk99Ri9wZrA6b64XROdMTzAilHiPio9bR6NwNtkG558 zYjCEUbVmsKpgcNsXiBEXdBqLicS9VyQDgEQIYcQtNGFtr1npgI/cBYz9piQ1+BR H6jlDndCeq5xZS2EfdIIo1XFohdIpQuwqKirqOK7pYPcA9Ii3Sg2Od+HbcKSa5hd 7e1LsJ6pm5ZurNrvu4YDuupNSoYAGwlZ0XD1sEXXu+LuChQAtysW1gn+P2ERtI7p mTrQhCiQEdqGhppl38eqw/sWdubgqByamfV0TZH65HQpNUHHjxZ4FqBzIXK4mHxM bojG5OSrqO2mc0SztKokl/GfYw9iazdQfCvxjjEVxw2zYGoArsOF0zSEMX1haeC9 G5lSIDm2K9lmiliEtakPyvFPiu+hYI/Yyu9pXJSl6m1ulYNy3nT8VyDlFH8jdmpc inXs0Wo1FM4xF5O5ZTTWpXlLbT81I+XCKeDhCZivyw2v7jgQqA0zAywSjg0XZyNE QW+zAWJOK8wv/YdhKG8aI09bgpYdE0MJ6SuA5SGdBX+BmUzaAtw= =+NQQ -----END PGP SIGNATURE----- --rRUgzLQ9bkcy/8hZ--