On Mon, 17 Oct 2022 19:57:54 +1100 David Gibson wrote:

At least two places in passt calculate ICMPv6 checksums, ndp() and tap_ip_send(). Add a helper to handle this calculation in both places. For future flexibility, the new helper takes parameters for the fields in the IPv6 pseudo-header, so an IPv6 header or pseudo-header doesn't need to be explicitly constructed. It also allows the ICMPv6 header and payload to be in separate buffers, although we don't use this yet.

Signed-off-by: David Gibson --- checksum.c | 27 +++++++++++++++++++++++++++ checksum.h | 7 +++++++ ndp.c | 5 +---- tap.c | 6 ++---- 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/checksum.c b/checksum.c index 56ad01e..0e207c8 100644 --- a/checksum.c +++ b/checksum.c @@ -52,6 +52,8 @@ #include #include

+#include + /** * sum_16b() - Calculate sum of 16-bit words * @buf: Input buffer @@ -105,6 +107,31 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init) return (uint16_t)~csum_fold(sum_16b(buf, len) + init); }

+/** + * csum_icmp6() - Calculate checksum for an ICMPv6 packet

"Calculate and set" ...?

+ * @icmp6hr: ICMPv6 header, initialized apart from checksum + * @saddr: IPv6 source address + * @daddr: IPv6 destination address + * @payload: ICMP packet payload + * @len: Length of @payload (not including ICMPv6 header) + */ +void csum_icmp6(struct icmp6hdr *icmp6hr, + const struct in6_addr *saddr, + const struct in6_addr *daddr,

I think: const struct in6_addr *saddr, const struct in6_addr *daddr, would be easier on eyes.

+ const void *payload, + size_t len) +{ + /* Partial checksum for the pseudo-IPv6 header */ + uint32_t psum = sum_16b(saddr, sizeof(*saddr)) + + sum_16b(daddr, sizeof(*daddr)) + + htons(len + sizeof(*icmp6hr)) + htons(IPPROTO_ICMPV6);

Maybe: uint32_t psum = sum_16b(saddr, sizeof(*saddr)) + sum_16b(daddr, sizeof(*daddr)) + htons(len + sizeof(*icmp6hr)) + htons(IPPROTO_ICMPV6); for me, it turns things from "sum a bunch of things" to "addresses and something else".

+ + icmp6hr->icmp6_cksum = 0; + /* Add in partial checksum for the ICMPv6 header alone */ + psum += sum_16b(icmp6hr, sizeof(*icmp6hr)); + icmp6hr->icmp6_cksum = csum_unaligned(payload, len, psum); +} + /** * csum_tcp4() - Calculate TCP checksum for IPv4 and set in place * @iph: Packet buffer, IP header diff --git a/checksum.h b/checksum.h index 5418406..2c72200 100644 --- a/checksum.h +++ b/checksum.h @@ -6,9 +6,16 @@ #ifndef CHECKSUM_H #define CHECKSUM_H

+struct icmp6hdr; + uint32_t sum_16b(const void *buf, size_t len); uint16_t csum_fold(uint32_t sum); uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init); +void csum_icmp6(struct icmp6hdr *ih, + const struct in6_addr *saddr, + const struct in6_addr *daddr, + const void *payload, + size_t len);

It looks a bit like Haskell. ;) I would really use the horizontal space we have.

void csum_tcp4(struct iphdr *iph); uint16_t csum(const void *buf, size_t len, uint32_t init);

diff --git a/ndp.c b/ndp.c index dec36a9..03f1d06 100644 --- a/ndp.c +++ b/ndp.c @@ -189,10 +189,7 @@ dns_done: ip6hr->saddr = c->ip6.addr_ll;

ip6hr->payload_len = htons(sizeof(*ihr) + len); - ip6hr->hop_limit = IPPROTO_ICMPV6; - ihr->icmp6_cksum = 0; - ihr->icmp6_cksum = csum_unaligned(ip6hr, sizeof(*ip6hr) + - sizeof(*ihr) + len, 0); + csum_icmp6(ihr, &ip6hr->saddr, &ip6hr->daddr, ihr + 1, len);

Nice to see this all going away!

ip6hr->version = 6; ip6hr->nexthdr = IPPROTO_ICMPV6; diff --git a/tap.c b/tap.c index 8b6d9bc..aafc92b 100644 --- a/tap.c +++ b/tap.c @@ -191,10 +191,8 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, } else if (proto == IPPROTO_ICMPV6) { struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1);

- ih->icmp6_cksum = 0; - ih->icmp6_cksum = csum_unaligned(ip6h, - len + sizeof(*ip6h), - 0); + csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr, + ih + 1, len - sizeof(*ih)); } ip6h->version = 6; ip6h->nexthdr = proto;

-- Stefano

On Tue, Oct 18, 2022 at 05:01:51AM +0200, Stefano Brivio wrote:

On Mon, 17 Oct 2022 19:57:55 +1100 David Gibson wrote:

...

Although tap_ip_send() is currently the only place calculating ICMPv4 checksums, create a helper function for symmetry with ICMPv6. For future flexibility it allows the ICMPv6 header and payload to be in separate buffers.

Signed-off-by: David Gibson --- checksum.c | 15 +++++++++++++++ checksum.h | 2 ++ tap.c | 4 +--- 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/checksum.c b/checksum.c index 0e207c8..c8b6b42 100644 --- a/checksum.c +++ b/checksum.c @@ -52,6 +52,7 @@ #include #include

+#include #include

/** @@ -107,6 +108,20 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init) return (uint16_t)~csum_fold(sum_16b(buf, len) + init); }

+/** + * csum_icmp4() - Calculate checksum for an ICMPv4 packet

"Calculate and set"?

Done.

By the way, there's no such thing as ICMPv4 -- it's ICMP.

Technically, yes, but I kind of wanted to make it clear at a glance that these are IPv4 specific functions. I'd also like to avoid the implication that v4 is the "normal" sort. I've changed from "ICMPv4" to "ICMP" in the comments, but I've left the '4's in the various names

...

+ * @icmp4hr: ICMPv4 header, initialized apart from checksum

...-ised, if you respin. For consistency, I would call this 'ih'.

...

+ * @payload: ICMPv4 packet payload + * @len: Length of @payload (not including ICMPv4 header) + */ +void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)

I guess csum_icmp() is preferable. Indeed, for TCP and UDP 'tcp4' and 'udp4' make sense because those are the same protocols over IPv4 and IPv6.

See above.

...

+{ + /* Partial checksum for ICMPv4 header alone */ + uint32_t hrsum = sum_16b(icmp4hr, sizeof(*icmp4hr));

A white line would be nice here.

Done.

I would call this psum (same as in csum_icmp6())

Changed to 'psum'.

or hdrsum, 'hr' isn't really used for "header" elsewhere.

Well.. except as a suffix, 'ihr' etc.

...

+ icmp4hr->checksum = 0; + icmp4hr->checksum = csum_unaligned(payload, len, hrsum); +} + /** * csum_icmp6() - Calculate checksum for an ICMPv6 packet * @icmp6hr: ICMPv6 header, initialized apart from checksum diff --git a/checksum.h b/checksum.h index 2c72200..ff95cf9 100644 --- a/checksum.h +++ b/checksum.h @@ -6,11 +6,13 @@ #ifndef CHECKSUM_H #define CHECKSUM_H

+struct icmphdr; struct icmp6hdr;

uint32_t sum_16b(const void *buf, size_t len); uint16_t csum_fold(uint32_t sum); uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init); +void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len); void csum_icmp6(struct icmp6hdr *ih, const struct in6_addr *saddr, const struct in6_addr *daddr, diff --git a/tap.c b/tap.c index aafc92b..f082901 100644 --- a/tap.c +++ b/tap.c @@ -148,9 +148,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, uh->check = 0; } else if (iph->protocol == IPPROTO_ICMP) { struct icmphdr *ih = (struct icmphdr *)(iph + 1); - - ih->checksum = 0; - ih->checksum = csum_unaligned(ih, len, 0); + csum_icmp4(ih, ih + 1, len - sizeof(*ih)); }

if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1) < 0)

-- 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

On Tue, Oct 18, 2022 at 05:02:31AM +0200, Stefano Brivio wrote:

On Mon, 17 Oct 2022 19:57:56 +1100 David Gibson wrote:

...

Add a helper for calculating UDP checksums when used over IPv6 For future flexibility, the new helper takes parameters for the fields in the IPv6 pseudo-header, so an IPv6 header or pseudo-header doesn't need to be explicitly constructed. It also allows the UDP header and payload to be in separate buffers, although we don't use this yet.

Signed-off-by: David Gibson --- checksum.c | 23 +++++++++++++++++++++++ checksum.h | 5 +++++ tap.c | 5 ++--- 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/checksum.c b/checksum.c index c8b6b42..0849fb1 100644 --- a/checksum.c +++ b/checksum.c @@ -52,6 +52,7 @@ #include #include

+#include #include #include

@@ -122,6 +123,28 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) icmp4hr->checksum = csum_unaligned(payload, len, hrsum); }

+/** + * csum_udp6() - Calculate checksum for a UDP over IPv6 packet

Calculate and set.

Done.

...

+ * @udp6hr: UDP header, initialized apart from checksum

-ised.

Done.

...

+ * @payload: UDP packet payload + * @len: Length of @payload (not including UDP header) + */ +void csum_udp6(struct udphdr *udp6hr, + const struct in6_addr *saddr, + const struct in6_addr *daddr,

You could use some horizontal space.

Done.

...

+ const void *payload, size_t len) +{ + /* Partial checksum for the pseudo-IPv6 header */ + uint32_t psum = sum_16b(saddr, sizeof(*saddr)) + + sum_16b(daddr, sizeof(*daddr)) + + htons(len + sizeof(*udp6hr)) + htons(IPPROTO_UDP);

Alignment:

uint32_t psum = sum_16b(saddr, sizeof(*saddr)) + sum_16b(daddr, sizeof(*daddr)) + htons(len + sizeof(*udp6hr)) + htons(IPPROTO_UDP);

Done.

...

+ udp6hr->check = 0; + /* Add in partial checksum for the UDP header alone */ + psum += sum_16b(udp6hr, sizeof(*udp6hr)); + udp6hr->check = csum_unaligned(payload, len, psum); +} + /** * csum_icmp6() - Calculate checksum for an ICMPv6 packet * @icmp6hr: ICMPv6 header, initialized apart from checksum diff --git a/checksum.h b/checksum.h index ff95cf9..1b9f48e 100644 --- a/checksum.h +++ b/checksum.h @@ -6,6 +6,7 @@ #ifndef CHECKSUM_H #define CHECKSUM_H

+struct udphdr; struct icmphdr; struct icmp6hdr;

@@ -13,6 +14,10 @@ uint32_t sum_16b(const void *buf, size_t len); uint16_t csum_fold(uint32_t sum); uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init); void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len); +void csum_udp6(struct udphdr *udp6hr, + const struct in6_addr *saddr, + const struct in6_addr *daddr, + const void *payload, size_t len);

Use some horizontal space.

Done.

...

void csum_icmp6(struct icmp6hdr *ih, const struct in6_addr *saddr, const struct in6_addr *daddr, diff --git a/tap.c b/tap.c index f082901..9c197cb 100644 --- a/tap.c +++ b/tap.c @@ -183,9 +183,8 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, } else if (proto == IPPROTO_UDP) { struct udphdr *uh = (struct udphdr *)(ip6h + 1);

- uh->check = 0; - uh->check = csum_unaligned(ip6h, len + sizeof(*ip6h), - 0); + csum_udp6(uh, &ip6h->saddr, &ip6h->daddr, + uh + 1, len - sizeof(*uh)); } else if (proto == IPPROTO_ICMPV6) { struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1);

-- 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

On Mon, 17 Oct 2022 19:57:57 +1100 David Gibson wrote:

At least two places in passt fill in UDP over IPv4 checksums, although since UDP checksums are optional with IPv4 that just amounts to storing a 0 (in tap_ip_send()) or leaving a 0 from an earlier initialization (in dhcp()). For consistency, add a helper for this "calculation".

Just for the heck of it, add the option (compile time disabled for now) to calculate real UDP checksums.

Signed-off-by: David Gibson --- checksum.c | 33 +++++++++++++++++++++++++++++++++ checksum.h | 3 +++ dhcp.c | 2 +- tap.c | 2 +- 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/checksum.c b/checksum.c index 0849fb1..72f1cfb 100644 --- a/checksum.c +++ b/checksum.c @@ -56,6 +56,11 @@ #include #include

+/* Checksums are optional for UDP over IPv4, so we usually just set + * them to 0. Change this 1 to calculate real UDP over IPv4 checksums

to 1

+ */ +#define UDP4_REAL_CHECKSUMS 0 + /** * sum_16b() - Calculate sum of 16-bit words * @buf: Input buffer @@ -109,6 +114,34 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init) return (uint16_t)~csum_fold(sum_16b(buf, len) + init); }

+/** + * csum_udp4() - Calculate checksum for a UDP over IPv4 packet

and set

+ * @udp4hr: UDP header, initialized apart from checksum + * @saddr: IPv4 source address + * @daddr: IPv4 destination address + * @payload: ICMPv4 packet payload + * @len: Length of @payload (not including UDP) + */ +void csum_udp4(struct udphdr *udp4hr, + in_addr_t saddr, in_addr_t daddr, + const void *payload, size_t len) +{ + /* UDP checksums are optional, so don't bother */ + udp4hr->check = 0; + + if (UDP4_REAL_CHECKSUMS) { + /* UNTESTED: if we did want real UDPv4 checksums, this + * is roughly what we'd need */ + uint32_t psum = csum_fold(htonl(saddr)) + + csum_fold(htonl(daddr)) + + htons(len + sizeof(*udp4hr)) + + htons(IPPROTO_UDP); + /* Add in partial checksum for the UDP header alone */ + psum += sum_16b(udp4hr, sizeof(*udp4hr)); + udp4hr->check = csum_unaligned(payload, len, psum); + } +} + /** * csum_icmp4() - Calculate checksum for an ICMPv4 packet * @icmp4hr: ICMPv4 header, initialized apart from checksum diff --git a/checksum.h b/checksum.h index 1b9f48e..a9502b9 100644 --- a/checksum.h +++ b/checksum.h @@ -13,6 +13,9 @@ struct icmp6hdr; uint32_t sum_16b(const void *buf, size_t len); uint16_t csum_fold(uint32_t sum); uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init); +void csum_udp4(struct udphdr *udp4hr, + in_addr_t saddr, in_addr_t daddr, + const void *payload, size_t len);

Horizontal space. -- Stefano

On Tue, Oct 18, 2022 at 05:03:49AM +0200, Stefano Brivio wrote:

On Mon, 17 Oct 2022 19:57:58 +1100 David Gibson wrote:

...

We calculate IPv4 header checksums in at least two places, in dhcp() and in tap_ip_send. Add a helper to handle this calculation in both places.

Signed-off-by: David Gibson --- checksum.c | 6 ++++++ checksum.h | 1 + dhcp.c | 3 +-- tap.c | 3 +-- 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/checksum.c b/checksum.c index 72f1cfb..f25a96a 100644 --- a/checksum.c +++ b/checksum.c @@ -114,6 +114,12 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init) return (uint16_t)~csum_fold(sum_16b(buf, len) + init); }

/** * csum_ip4_header() - Calculate and set IPv4 header checksum * @iph: IPv4 header */

...I just tried to run Doxygen, I think it would be nice to have eventually (especially for DOT call graphs), things don't look too bad.

...

+void csum_ip4_header(struct iphdr *ip4hr) +{ + ip4hr->check = 0; + ip4hr->check = csum_unaligned(ip4hr, (size_t)ip4hr->ihl * 4, 0);

iph, for consistency.

As noted before, I'd prefer to avoid the implication that IPv4 is normal and IPv6 is special. I have changed to just ip4h, though. -- 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

On Mon, 17 Oct 2022 19:57:59 +1100 David Gibson wrote:

tap_ip_send() doesn't take a destination address, because it's specifically for inbound packets, and the IP addresses of the guest/namespace are already known to us. Rather than open-coding this destination address logic, make helper functions for it which will enable some later cleanups.

Signed-off-by: David Gibson --- tap.c | 29 ++++++++++++++++++++++++----- tap.h | 3 +++ 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/tap.c b/tap.c index de02c56..41e8ff2 100644 --- a/tap.c +++ b/tap.c @@ -96,6 +96,28 @@ int tap_send(const struct ctx *c, const void *data, size_t len, int vnet_pre) return write(c->fd_tap, (char *)data + (vnet_pre ? 4 : 0), len); }

+/** + * tap_ip4_daddr() - Normal IPv4 destination address for inbound packets + * @c: Execution context

Given that the address is returned in network order, I think this would be relevant here: * Return: IPv4 address, network order

+ */ +in_addr_t tap_ip4_daddr(const struct ctx *c) +{ + return c->ip4.addr_seen; +} + +/** + * tap_ip6_daddr() - Normal IPv4 destination address for inbound packets + * @c: Execution context + * @src: Source address

* Return: pointer to IPv6 address

+ */ +const struct in6_addr *tap_ip6_daddr(const struct ctx *c, + const struct in6_addr *src) +{ + if (IN6_IS_ADDR_LINKLOCAL(src)) + return &c->ip6.addr_ll_seen; + return &c->ip6.addr_seen; +} + /** * tap_ip_send() - Send IP packet, with L2 headers, calculating L3/L4 checksums * @c: Execution context @@ -132,7 +154,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, iph->frag_off = 0; iph->ttl = 255; iph->protocol = proto; - iph->daddr = c->ip4.addr_seen; + iph->daddr = tap_ip4_daddr(c); memcpy(&iph->saddr, &src->s6_addr[12], 4);

csum_ip4_header(iph); @@ -163,10 +185,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, ip6h->priority = 0;

ip6h->saddr = *src; - if (IN6_IS_ADDR_LINKLOCAL(src)) - ip6h->daddr = c->ip6.addr_ll_seen; - else - ip6h->daddr = c->ip6.addr_seen; + ip6h->daddr = *tap_ip6_daddr(c, src);

memcpy(data, in, len);

diff --git a/tap.h b/tap.h index df3aec0..a6764b4 100644 --- a/tap.h +++ b/tap.h @@ -6,6 +6,9 @@ #ifndef TAP_H #define TAP_H

+in_addr_t tap_ip4_daddr(const struct ctx *c); +const struct in6_addr *tap_ip6_daddr(const struct ctx *c, + const struct in6_addr *src); void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, const char *in, size_t len, uint32_t flow); int tap_send(const struct ctx *c, const void *data, size_t len, int vnet_pre);

-- Stefano

On Tue, Oct 18, 2022 at 05:06:11AM +0200, Stefano Brivio wrote:

On Mon, 17 Oct 2022 19:58:02 +1100 David Gibson wrote:

...

The IPv4 and IPv6 paths in tap_ip_send() have very little in common, and it turns out that every caller (statically) knows if it is using IPv4 or IPv6. So split into separate tap_ip4_send() and tap_ip6_send() functions. Use a new tap_l2_hdr() function for the very small common part.

While we're there, make some minor cleanups: - We were double writing some fields in the IPv6 header, so that it temporary matched the pseudo-header for checksum calculation. With recent checksum reworks, this isn't neccessary any more. - We don't use any IPv4 header options, so use some sizeof() constructs instead of some open coded values for header length. - The comment used to say that the flow label was for TCP over IPv6, but in fact the only thing we used it for was ICMPv6

...right, this used to be the data path.

Makes sense. Realized that comment isn't quite correct, because it was DHCPv6 rather than ICMPv6 traffic that got the flow labels.

...

Signed-off-by: David Gibson --- dhcpv6.c | 6 +- icmp.c | 10 +--- tap.c | 176 +++++++++++++++++++++++++++++-------------------------- tap.h | 6 +- 4 files changed, 102 insertions(+), 96 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c index e7640ce..7829968 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -531,8 +531,8 @@ int dhcpv6(struct ctx *c, const struct pool *p,

resp_not_on_link.hdr.xid = mh->xid;

- tap_ip_send(c, src, IPPROTO_UDP, - (char *)&resp_not_on_link, n, mh->xid); + tap_ip6_send(c, src, IPPROTO_UDP, + (char *)&resp_not_on_link, n, mh->xid);

return 1; } @@ -580,7 +580,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,

resp.hdr.xid = mh->xid;

- tap_ip_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid); + tap_ip6_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid); c->ip6.addr_seen = c->ip6.addr;

return 1; diff --git a/icmp.c b/icmp.c index 21ea2d7..61c2d90 100644 --- a/icmp.c +++ b/icmp.c @@ -69,10 +69,6 @@ static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)]; void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now) { - struct in6_addr a6 = { .s6_addr = { 0, 0, 0, 0, - 0, 0, 0, 0, - 0, 0, 0xff, 0xff, - 0, 0, 0, 0 } }; union icmp_epoll_ref *iref = &ref.r.p.icmp; struct sockaddr_storage sr; socklen_t sl = sizeof(sr); @@ -109,7 +105,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, icmp_id_map[V6][id].seq = seq; }

- tap_ip_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0); + tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0); } else { struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr; struct icmphdr *ih = (struct icmphdr *)buf; @@ -127,9 +123,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, icmp_id_map[V4][id].seq = seq; }

- memcpy(&a6.s6_addr[12], &sr4->sin_addr, sizeof(sr4->sin_addr)); - - tap_ip_send(c, &a6, IPPROTO_ICMP, buf, n, 0); + tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n); } }

diff --git a/tap.c b/tap.c index ae75fac..45547ac 100644 --- a/tap.c +++ b/tap.c @@ -109,100 +109,110 @@ const struct in6_addr *tap_ip6_daddr(const struct ctx *c, }

/** - * tap_ip_send() - Send IP packet, with L2 headers, calculating L3/L4 checksums + * tap_l2_hdr() - Build an L2 header for an inbound packet * @c: Execution context - * @src: IPv6 source address, IPv4-mapped for IPv4 sources - * @proto: L4 protocol number - * @in: Payload - * @len: L4 payload length - * @flow: Flow label for TCP over IPv6 + * @buf: Buffer address at which to generate header + * @proto: Ethernet protocol number for L3 + * + * Returns a pointer at which to write the packet's payload

* Return: ...

Done. -- 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

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:

...

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 suitable helper we can split it into tap_udp4_send() and tap_icmp4_send() functions without greatly increasing the code size, this removing that layering violation.

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.

Signed-off-by: David Gibson --- icmp.c | 3 ++- tap.c | 75 +++++++++++++++++++++++++++++++++++++++++----------------- tap.h | 7 ++++-- 3 files changed, 60 insertions(+), 25 deletions(-)

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 epoll_ref ref, icmp_id_map[V4][id].seq = seq; }

- 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); } }

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; }

-/** - * 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)

I understand why you return ip(4)h + 1 here because I've just reviewed 9/14, I wouldn't know otherwise:

/** * 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 = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP); - char *data = (char *)(ip4h + 1); + struct iphdr *ip4h = (struct iphdr *)buf;

ip4h->version = 4; ip4h->ihl = 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 = 255; ip4h->protocol = proto; ip4h->saddr = src; - ip4h->daddr = tap_ip4_daddr(c); + ip4h->daddr = 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 = len + sizeof(struct udphdr); + char buf[USHRT_MAX]; + void *ip4h = tap_l2_hdr(c, buf, ETH_P_IP); + void *uhp = tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP);

Two observations:

- 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.

I would rather have to +1 the return value or the original pointer instead or having this trick

...

+ struct udphdr *uh = (struct udphdr *)uhp; + char *data = (char *)(uh + 1);

- 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;

iph = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP) + 1; tap_ip_hdr(iph, src, dst, len + sizeof(uh), IPPROTO_UDP);

uh = (struct udphdr *)iph + 1; uh->source = htons(sport); uh->dest = htons(dport); uh->len = htons(len + sizeof(uh)); csum_udp4(uh, src, dst, in, len);

data = uh + 1; memcpy(data, in, len);

if (tap_send(c, buf, len + (data - buf)) < 0) debug("tap: failed to send %lu bytes (IPv4)", len);

...

+ uh->source = htons(sport); + uh->dest = htons(dport); + uh->len = htons(udplen); + csum_udp4(uh, src, dst, in, len); memcpy(data, in, len);

- if (ip4h->protocol == IPPROTO_UDP) { - struct udphdr *uh = (struct udphdr *)(ip4h + 1); + if (tap_send(c, buf, len + (data - buf)) < 0) + debug("tap: failed to send %lu bytes (IPv4)", len); +}

- csum_udp4(uh, ip4h->saddr, ip4h->daddr, - uh + 1, len - sizeof(*uh)); - } else if (ip4h->protocol == IPPROTO_ICMP) { - struct icmphdr *ih = (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 = tap_l2_hdr(c, buf, ETH_P_IP); + char *data = tap_ip4_hdr(ip4h, src, dst, len, IPPROTO_ICMP); + struct icmphdr *icmp4h = (struct icmphdr *)data;

...same here, even though perhaps not so apparent.

...

+ + memcpy(data, in, len); + csum_icmp4(icmp4h, icmp4h + 1, len - sizeof(*icmp4h));

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

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,

-- 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

On Tue, Oct 18, 2022 at 02:27:04PM +0200, Stefano Brivio wrote:

On Tue, 18 Oct 2022 23:07:58 +1100 David Gibson wrote:

...

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:

...

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 suitable helper we can split it into tap_udp4_send() and tap_icmp4_send() functions without greatly increasing the code size, this removing that layering violation.

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.

Signed-off-by: David Gibson --- icmp.c | 3 ++- tap.c | 75 +++++++++++++++++++++++++++++++++++++++++----------------- tap.h | 7 ++++-- 3 files changed, 60 insertions(+), 25 deletions(-)

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 epoll_ref ref, icmp_id_map[V4][id].seq = seq; }

- 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); } }

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; }

-/** - * 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)

I understand why you return ip(4)h + 1 here because I've just reviewed 9/14, I wouldn't know otherwise:

/** * 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 = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP); - char *data = (char *)(ip4h + 1); + struct iphdr *ip4h = (struct iphdr *)buf;

ip4h->version = 4; ip4h->ihl = 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 = 255; ip4h->protocol = proto; ip4h->saddr = src; - ip4h->daddr = tap_ip4_daddr(c); + ip4h->daddr = 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 = len + sizeof(struct udphdr); + char buf[USHRT_MAX]; + void *ip4h = tap_l2_hdr(c, buf, ETH_P_IP); + void *uhp = tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP);

Two observations:

- 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.

I would rather have to +1 the return value or the original pointer instead or having this trick

...

+ struct udphdr *uh = (struct udphdr *)uhp; + char *data = (char *)(uh + 1);

- 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.

Hmm, yes, I think the variable length case is a very valid point, and also in terms of abstraction I see the advantage. There are just two things I can think of:

- passing the end pointer as argument (not as practical as your solution, though)

- naming it tap_ip4_push_hdr(), tap_ip4_hdr_after(), tap_ip4_hdr_goto_next(), tap_ip4_leave_header_behind()... I can't think of anything better at this point. I'll keep thinking, but at the moment I'd be fine even with the current name.

I've gone with a variant of the 'push' naming, I think that makes it a bit clearer. -- 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