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 8F4F75A0276 for ; Tue, 18 Oct 2022 14:08:17 +0200 (CEST) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4MsCLH5876z4xGw; Tue, 18 Oct 2022 23:08:07 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1666094887; bh=owQG7ek2QmvKQA7+IXhl/9pi3TtNL2C/2GRV7hxOyKM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=P/ZGAQKAbwScdUJbZpX+HJU22KoNS7P5w3FqksvY5tcLp88Io5gWNXLa1dKppqbgD tziZbaNy6bQw/+NzOWW47ViWwmdlT4C2KQAn73nKvqsAoXzGMVI92sAKc1adAmC1OD BkkrAWzkc6k6M+CRoBZw2aQPk58oGS5khhMJSSFM= Date: Tue, 18 Oct 2022 23:07:58 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 13/14] tap: Split tap_ip4_send() into UDP and ICMP variants Message-ID: References: <20221017085807.473470-1-david@gibson.dropbear.id.au> <20221017085807.473470-14-david@gibson.dropbear.id.au> <20221018050634.428cf8d6@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="O/nBYc7pvFMyGTr0" Content-Disposition: inline In-Reply-To: <20221018050634.428cf8d6@elisabeth> Message-ID-Hash: MMM2BFP2GEYFVOJRMG6VIRLNT3LOIXVN X-Message-ID-Hash: MMM2BFP2GEYFVOJRMG6VIRLNT3LOIXVN 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.3 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: --O/nBYc7pvFMyGTr0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 18, 2022 at 05:06:34AM +0200, Stefano Brivio wrote: > On Mon, 17 Oct 2022 19:58:06 +1100 > David Gibson wrote: >=20 > > tap_ip4_send() has special case logic to compute the checksums for UDP > > and ICMP packets, which is a mild layering violation. By using a suita= ble > > helper we can split it into tap_udp4_send() and tap_icmp4_send() functi= ons > > without greatly increasing the code size, this removing that layering > > violation. > >=20 > > We make some small changes to the interface while there. In both cases > > we make the destination IPv4 address a parameter, which will be useful > > later. For the UDP variant we make it take just the UDP payload, and it > > will generate the UDP header. For the ICMP variant we pass in the ICMP > > header as before. The inconsistency is because that's what seems to be > > the more natural way to invoke the function in the callers in each case. > >=20 > > Signed-off-by: David Gibson > > --- > > icmp.c | 3 ++- > > tap.c | 75 +++++++++++++++++++++++++++++++++++++++++----------------- > > tap.h | 7 ++++-- > > 3 files changed, 60 insertions(+), 25 deletions(-) > >=20 > > diff --git a/icmp.c b/icmp.c > > index 6493ea9..233acf9 100644 > > --- a/icmp.c > > +++ b/icmp.c > > @@ -124,7 +124,8 @@ void icmp_sock_handler(const struct ctx *c, union e= poll_ref ref, > > icmp_id_map[V4][id].seq =3D seq; > > } > > =20 > > - tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n); > > + tap_icmp4_send(c, sr4->sin_addr.s_addr, tap_ip4_daddr(c), > > + buf, n); > > } > > } > > =20 > > diff --git a/tap.c b/tap.c > > index 274f4ba..5792880 100644 > > --- a/tap.c > > +++ b/tap.c > > @@ -127,20 +127,10 @@ static void *tap_l2_hdr(const struct ctx *c, void= *buf, uint16_t proto) > > return eh + 1; > > } > > =20 > > -/** > > - * tap_ip4_send() - Send IPv4 packet, with L2 headers, calculating L3/= L4 checksums > > - * @c: Execution context > > - * @src: IPv4 source address > > - * @proto: L4 protocol number > > - * @in: Payload > > - * @len: L4 payload length > > - */ > > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > > - const char *in, size_t len) >=20 > I understand why you return ip(4)h + 1 here because I've just reviewed > 9/14, I wouldn't know otherwise: >=20 > /** > * tap_ip4_hdr() - Build IPv4 header for inbound packet, with checksum > * @c: Execution context > * @src: IPv4 source address, network order > * @dst: IPv4 destination address, network order > * @len: L4 payload length > * @proto: L4 protocol number > * > * Return: pointer to write payload to > */ Oops, yes, forgot to add a function comment. Done. > > +static void *tap_ip4_hdr(char *buf, in_addr_t src, in_addr_t dst, > > + size_t len, uint8_t proto) > > { > > - char buf[USHRT_MAX]; > > - struct iphdr *ip4h =3D (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP); > > - char *data =3D (char *)(ip4h + 1); > > + struct iphdr *ip4h =3D (struct iphdr *)buf; > > =20 > > ip4h->version =3D 4; > > ip4h->ihl =3D sizeof(struct iphdr) / 4; > > @@ -151,20 +141,61 @@ void tap_ip4_send(const struct ctx *c, in_addr_t = src, uint8_t proto, > > ip4h->ttl =3D 255; > > ip4h->protocol =3D proto; > > ip4h->saddr =3D src; > > - ip4h->daddr =3D tap_ip4_daddr(c); > > + ip4h->daddr =3D dst; > > csum_ip4_header(ip4h); > > + return ip4h + 1; > > +} > > + > > +/** > > + * tap_udp4_send() - Send UDP over IPv4 packet > > + * @c: Execution context > > + * @src: IPv4 source address > > + * @sport: UDP source port > > + * @dst: IPv4 destination address > > + * @dport: UDP destination port > > + * @in: UDP payload contents (not including UDP header) > > + * @len: UDP payload length (not including UDP header) > > + */ > > +/* cppcheck-suppress unusedFunction */ > > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t sport, > > + in_addr_t dst, in_port_t dport, > > + const void *in, size_t len) > > +{ > > + size_t udplen =3D len + sizeof(struct udphdr); > > + char buf[USHRT_MAX]; > > + void *ip4h =3D tap_l2_hdr(c, buf, ETH_P_IP); > > + void *uhp =3D tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP); >=20 > Two observations: >=20 > - this saves one line and one cast, but it's really a bit unnatural that > tap_ip4_hdr() doesn't point to the header it just made, or to nothing. >=20 > I would rather have to +1 the return value or the original pointer > instead or having this trick >=20 > > + struct udphdr *uh =3D (struct udphdr *)uhp; > > + char *data =3D (char *)(uh + 1); >=20 > - it's longer, but in my opinion clearer, if we split a bit more clearly > the components of the packet, that is, something like (untested): I don't really want to change this. Yes, it's a bit counterintuitive at first blush, but there's a reason for this approach. This style of a function which generates a header then points *after* it works even if the header it generates is of variable length. Advancing to the payload in the caller doesn't (at least not without breaking the abstraction I'm trying to generate with these helpers). That's not just theoretical, because at some point I'd like to extend the l2_hdr function to also allocate space for the qemu socket length header. I'm certainly open to name changes to make this behaviour more obvious, but I think returning the payload pointer not the header pointer makes for a better abstraction here. > char buf[USHRT_MAX]; > struct udphdr *uh; > struct iphdr *iph; > char *data; >=20 > iph =3D (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP) + 1; > tap_ip_hdr(iph, src, dst, len + sizeof(uh), IPPROTO_UDP); >=20 > uh =3D (struct udphdr *)iph + 1; > uh->source =3D htons(sport); > uh->dest =3D htons(dport); > uh->len =3D htons(len + sizeof(uh)); > csum_udp4(uh, src, dst, in, len); >=20 > data =3D uh + 1; > memcpy(data, in, len); >=20 > if (tap_send(c, buf, len + (data - buf)) < 0) > debug("tap: failed to send %lu bytes (IPv4)", len); > > =20 > > + uh->source =3D htons(sport); > > + uh->dest =3D htons(dport); > > + uh->len =3D htons(udplen); > > + csum_udp4(uh, src, dst, in, len); > > memcpy(data, in, len); > > =20 > > - if (ip4h->protocol =3D=3D IPPROTO_UDP) { > > - struct udphdr *uh =3D (struct udphdr *)(ip4h + 1); > > + if (tap_send(c, buf, len + (data - buf)) < 0) > > + debug("tap: failed to send %lu bytes (IPv4)", len); > > +} > > =20 > > - csum_udp4(uh, ip4h->saddr, ip4h->daddr, > > - uh + 1, len - sizeof(*uh)); > > - } else if (ip4h->protocol =3D=3D IPPROTO_ICMP) { > > - struct icmphdr *ih =3D (struct icmphdr *)(ip4h + 1); > > - csum_icmp4(ih, ih + 1, len - sizeof(*ih)); > > - } > > +/** > > + * tap_icmp4_send() - Send ICMPv4 packet > > + * @c: Execution context > > + * @src: IPv4 source address > > + * @dst: IPv4 destination address > > + * @in: ICMP packet, including ICMP header > > + * @len: ICMP packet length, including ICMP header > > + */ > > +void tap_icmp4_send(const struct ctx *c, in_addr_t src, in_addr_t dst, > > + void *in, size_t len) > > +{ > > + char buf[USHRT_MAX]; > > + void *ip4h =3D tap_l2_hdr(c, buf, ETH_P_IP); > > + char *data =3D tap_ip4_hdr(ip4h, src, dst, len, IPPROTO_ICMP); > > + struct icmphdr *icmp4h =3D (struct icmphdr *)data; >=20 > ...same here, even though perhaps not so apparent. >=20 > > + > > + memcpy(data, in, len); > > + csum_icmp4(icmp4h, icmp4h + 1, len - sizeof(*icmp4h)); > > =20 > > if (tap_send(c, buf, len + (data - buf)) < 0) > > debug("tap: failed to send %lu bytes (IPv4)", len); > > diff --git a/tap.h b/tap.h > > index d43c7a0..743bc58 100644 > > --- a/tap.h > > +++ b/tap.h > > @@ -7,10 +7,13 @@ > > #define TAP_H > > =20 > > in_addr_t tap_ip4_daddr(const struct ctx *c); > > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t sport, > > + in_addr_t dst, in_port_t dport, > > + const void *in, size_t len); > > +void tap_icmp4_send(const struct ctx *c, in_addr_t src, in_addr_t dst, > > + void *in, size_t len); > > const struct in6_addr *tap_ip6_daddr(const struct ctx *c, > > const struct in6_addr *src); > > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > > - const char *in, size_t len); > > void tap_udp6_send(const struct ctx *c, > > const struct in6_addr *src, in_port_t sport, > > const struct in6_addr *dst, in_port_t dport, >=20 --=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 --O/nBYc7pvFMyGTr0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmNOlxcACgkQgypY4gEw YSKQGxAAmRBWIhVfvK/PFAlVEKxGhPyY8x4qxs5D9wG+5gHtfj4PlVRAgVphsT18 G33tIBdKvXg3kb8rWRD7hkC/rDm/TJfWBe7s8CZPja06QJav/EJfvQYtx3i9OmBb Qn+glfV81fSIoDTRpJ5LEuYuUi1jjmx8vh/QcI4pF/bWwiIDg2JIXKjpQvq6X/2j jgjH68MH8o6FYcjt/5dFyPMS+5nr227psU6N60xFB24s15UzYQAfN6p+3PwrEO8Q IBF15CsImqub9Nifd3bWwySaTybWSrAA3oGU2swYyZjkCGDvarv3/wFymu06Hcgd 2XIpEoWKQFE4onjGRqMu5JCMBpxwQCfztON+8abUmsG4jfahvaS3odcmDtteuTFt U11lEGA4iS4jgcci6Qm7t05aPkZfsjgx8Q+XaBbU3xttuk3X3gSEdkSfjoGdBESC s3pmvaz736AQqsi+ra5nGbZZWG2iseGEwlxSGM98cphqGUZrCDEhWDCTnoGBMMEb 6/kI9oe4iT1OaPg/ErzLdPMPFbd4Nlts2AQW4OA4Rz46MXcQor9WFUEjJhyGIMhy HNsRSI1V8XWDSzmzhGBx+oE4UVVt7R8kVt8U8OBeeqomCCXGspWerlYwzGY+GfyP ipEGhEjnTyae8J/DajaVRvY4Qw9CUZPIeEEA+PHaqB5fJA210xg= =Yyl3 -----END PGP SIGNATURE----- --O/nBYc7pvFMyGTr0--