On Wed, 19 Oct 2022 11:43:53 +1100 David Gibson wrote:

tap_ip6_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_udp6_send() and tap_icmp6_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 IPv6 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 --- dhcpv6.c | 21 +++------------ icmp.c | 3 ++- tap.c | 82 ++++++++++++++++++++++++++++++++++++++++++-------------- tap.h | 9 +++++-- 4 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c index 7829968..e763aed 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -208,15 +208,8 @@ struct msg_hdr { uint32_t xid:24; } __attribute__((__packed__));

-#if __BYTE_ORDER == __BIG_ENDIAN -#define UH_RESP {{{ 547, 546, 0, 0, }}} -#else -#define UH_RESP {{{ __bswap_constant_16(547), __bswap_constant_16(546), 0, 0 }}} -#endif - /** * struct resp_t - Normal advertise and reply message - * @uh: UDP header * @hdr: DHCP message header * @server_id: Server Identifier option * @ia_na: Non-temporary Address option @@ -226,7 +219,6 @@ struct msg_hdr { * @dns_search: Domain Search List, here just for storage size */ static struct resp_t { - struct udphdr uh; struct msg_hdr hdr;

struct opt_server_id server_id; @@ -236,7 +228,6 @@ static struct resp_t { struct opt_dns_servers dns_servers; struct opt_dns_search dns_search; } __attribute__((__packed__)) resp = { - UH_RESP, { 0 }, SERVER_ID,

@@ -270,13 +261,11 @@ static const struct opt_status_code sc_not_on_link = {

/** * struct resp_not_on_link_t - NotOnLink error (mandated by RFC 8415, 18.3.2.) - * @uh: UDP header * @hdr: DHCP message header * @server_id: Server Identifier option * @var: Payload: IA_NA from client, status code, client ID */ static struct resp_not_on_link_t { - struct udphdr uh; struct msg_hdr hdr;

struct opt_server_id server_id; @@ -284,7 +273,6 @@ static struct resp_not_on_link_t { uint8_t var[sizeof(struct opt_ia_na) + sizeof(struct opt_status_code) + sizeof(struct opt_client_id)]; } __attribute__((__packed__)) resp_not_on_link = { - UH_RESP, { TYPE_REPLY, 0 }, SERVER_ID, { 0, }, @@ -527,12 +515,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, n += sizeof(struct opt_hdr) + ntohs(client_id->l);

n = offsetof(struct resp_not_on_link_t, var) + n; - resp_not_on_link.uh.len = htons(n);

resp_not_on_link.hdr.xid = mh->xid;

- tap_ip6_send(c, src, IPPROTO_UDP, - (char *)&resp_not_on_link, n, mh->xid); + tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546, + mh->xid, &resp_not_on_link, n);

return 1; } @@ -576,11 +563,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, n = offsetof(struct resp_t, client_id) + sizeof(struct opt_hdr) + ntohs(client_id->l); n = dhcpv6_dns_fill(c, (char *)&resp, n); - resp.uh.len = htons(n);

resp.hdr.xid = mh->xid;

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

return 1; diff --git a/icmp.c b/icmp.c index 61c2d90..6493ea9 100644 --- a/icmp.c +++ b/icmp.c @@ -105,7 +105,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, icmp_id_map[V6][id].seq = seq; }

- tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0); + tap_icmp6_send(c, &sr6->sin6_addr, + tap_ip6_daddr(c, &sr6->sin6_addr), buf, n); } else { struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr; struct icmphdr *ih = (struct icmphdr *)buf; diff --git a/tap.c b/tap.c index 0e8c99b..135d799 100644 --- a/tap.c +++ b/tap.c @@ -175,21 +175,22 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, }

/** - * tap_ip6_send() - Send IPv6 packet, with L2 headers, calculating L3/L4 checksums + * tap_push_ip6h() - Build IPv6 header for inbound packet * @c: Execution context * @src: IPv6 source address - * @proto: L4 protocol number - * @in: Payload + * @dst: IPv6 destination address * @len: L4 payload length - * @flow: Flow label + * @proto: L4 protocol number + * @flow: IPv6 flow identifier + * + * Return: pointer at which to write the packet's payload */ -void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, - uint8_t proto, const char *in, size_t len, uint32_t flow) +static void *tap_push_ip6h(char *buf,

Sorry, I missed this during review, but this function doesn't take @c anymore, and the comment change didn't cover all the argument changes. It takes a buffer pointer which is not entirely obvious. You changed this later in 1ebe787fe460 ("tap: Simplify some casts in the tap "slow path" functions"), but that commit doesn't update the comment either. Same for tap_push_ip4h(). I just realised as I accidentally passed an "input" header to it.

+ const struct in6_addr *src, + const struct in6_addr *dst, + size_t len, uint8_t proto, uint32_t flow) { - char buf[USHRT_MAX]; - struct ipv6hdr *ip6h = - (struct ipv6hdr *)tap_push_l2h(c, buf, ETH_P_IPV6); - char *data = (char *)(ip6h + 1); + struct ipv6hdr *ip6h = (struct ipv6hdr *)buf;

ip6h->payload_len = htons(len); ip6h->priority = 0; @@ -197,24 +198,65 @@ void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, ip6h->nexthdr = proto; ip6h->hop_limit = 255; ip6h->saddr = *src; - ip6h->daddr = *tap_ip6_daddr(c, src); + ip6h->daddr = *dst; ip6h->flow_lbl[0] = (flow >> 16) & 0xf; ip6h->flow_lbl[1] = (flow >> 8) & 0xff; ip6h->flow_lbl[2] = (flow >> 0) & 0xff; + return ip6h + 1; +}

+/** + * tap_udp6_send() - Send UDP over IPv6 packet + * @c: Execution context + * @src: IPv6 source address + * @sport: UDP source port + * @dst: IPv6 destination address + * @dport: UDP destination port + * @flow: Flow label + * @in: UDP payload contents (not including UDP header) + * @len: UDP payload length (not including UDP header) + */ +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, + uint32_t flow, const void *in, size_t len) +{ + size_t udplen = len + sizeof(struct udphdr); + char buf[USHRT_MAX]; + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); + void *uhp = tap_push_ip6h(ip6h, src, dst, udplen, IPPROTO_UDP, flow); + struct udphdr *uh = (struct udphdr *)uhp; + char *data = (char *)(uh + 1); + + uh->source = htons(sport); + uh->dest = htons(dport); + uh->len = htons(udplen); + csum_udp6(uh, src, dst, in, len); memcpy(data, in, len);

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

- csum_udp6(uh, &ip6h->saddr, &ip6h->daddr, - uh + 1, len - sizeof(*uh)); - } else if (proto == IPPROTO_ICMPV6) { - struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1); +/** + * tap_icmp6_send() - Send ICMPv6 packet + * @c: Execution context + * @src: IPv6 source address + * @dst: IPv6 destination address + * @in: ICMP packet, including ICMP header + * @len: ICMP packet length, including ICMP header + */ +void tap_icmp6_send(const struct ctx *c, + const struct in6_addr *src, const struct in6_addr *dst, + void *in, size_t len) +{ + char buf[USHRT_MAX]; + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); + char *data = tap_push_ip6h(ip6h, src, dst, len, IPPROTO_ICMPV6, 0); + struct icmp6hdr *icmp6h = (struct icmp6hdr *)data;

- csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr, - ih + 1, len - sizeof(*ih)); - } + memcpy(data, in, len); + csum_icmp6(icmp6h, src, dst, icmp6h + 1, len - sizeof(*icmp6h));

if (tap_send(c, buf, len + (data - buf)) < 1) debug("tap: failed to send %lu bytes (IPv6)", len); diff --git a/tap.h b/tap.h index 011ba8e..d43c7a0 100644 --- a/tap.h +++ b/tap.h @@ -11,8 +11,13 @@ 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_ip6_send(const struct ctx *c, const struct in6_addr *src, - uint8_t proto, const char *in, size_t len, uint32_t flow); +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, + uint32_t flow, const void *in, size_t len); +void tap_icmp6_send(const struct ctx *c, + const struct in6_addr *src, const struct in6_addr *dst, + void *in, size_t len); int tap_send(const struct ctx *c, const void *data, size_t len); void tap_handler(struct ctx *c, int fd, uint32_t events, const struct timespec *now);

-- Stefano

Although tap_ip_send() is currently the only place calculating ICMP 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 | 16 ++++++++++++++++ checksum.h | 2 ++ tap.c | 4 +--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/checksum.c b/checksum.c index 78c6960..f35c948 100644 --- a/checksum.c +++ b/checksum.c @@ -52,6 +52,7 @@ #include #include +#include #include /** @@ -107,6 +108,21 @@ 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 and set checksum for an ICMP packet + * @icmp4hr: ICMP header, initialised apart from checksum + * @payload: ICMP packet payload + * @len: Length of @payload (not including ICMP header) + */ +void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) +{ + /* Partial checksum for ICMP header alone */ + uint32_t psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + + icmp4hr->checksum = 0; + icmp4hr->checksum = csum_unaligned(payload, len, psum); +} + /** * csum_icmp6() - Calculate and set checksum for an ICMPv6 packet * @icmp6hr: ICMPv6 header, initialised apart from checksum diff --git a/checksum.h b/checksum.h index d7daabf..bf0620f 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 *icmp6hr, const struct in6_addr *saddr, const struct in6_addr *daddr, const void *payload, size_t len); 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) -- 2.37.3

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 | 2 ++ dhcp.c | 2 +- tap.c | 2 +- 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/checksum.c b/checksum.c index 175381d..cf6fc31 100644 --- a/checksum.c +++ b/checksum.c @@ -56,6 +56,12 @@ #include #include +/* Checksums are optional for UDP over IPv4, so we usually just set + * them to 0. Change this to 1 to calculate real UDP over IPv4 + * checksums + */ +#define UDP4_REAL_CHECKSUMS 0 + /** * sum_16b() - Calculate sum of 16-bit words * @buf: Input buffer @@ -109,6 +115,33 @@ 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 and set checksum for a UDP over IPv4 packet + * @udp4hr: UDP header, initialised 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 and set checksum for an ICMP packet * @icmp4hr: ICMP header, initialised apart from checksum diff --git a/checksum.h b/checksum.h index 2bb2ff9..2a5e915 100644 --- a/checksum.h +++ b/checksum.h @@ -13,6 +13,8 @@ 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); 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, diff --git a/dhcp.c b/dhcp.c index 7f0cc0b..8dcf645 100644 --- a/dhcp.c +++ b/dhcp.c @@ -364,9 +364,9 @@ int dhcp(const struct ctx *c, const struct pool *p) opt_set_dns_search(c, sizeof(m->o)); uh->len = htons(len = offsetof(struct msg, o) + fill(m) + sizeof(*uh)); - uh->check = 0; uh->source = htons(67); uh->dest = htons(68); + csum_udp4(uh, c->ip4.gw, c->ip4.addr, uh + 1, len - sizeof(*uh)); iph->tot_len = htons(len += sizeof(*iph)); iph->daddr = c->ip4.addr; diff --git a/tap.c b/tap.c index 9c197cb..58fc1de 100644 --- a/tap.c +++ b/tap.c @@ -145,7 +145,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, } else if (iph->protocol == IPPROTO_UDP) { struct udphdr *uh = (struct udphdr *)(iph + 1); - uh->check = 0; + csum_udp4(uh, iph->saddr, iph->daddr, uh + 1, len - sizeof(*uh)); } else if (iph->protocol == IPPROTO_ICMP) { struct icmphdr *ih = (struct icmphdr *)(iph + 1); csum_icmp4(ih, ih + 1, len - sizeof(*ih)); -- 2.37.3

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 | 33 ++++++++++++++++++++++++++++----- tap.h | 3 +++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/tap.c b/tap.c index de02c56..89be383 100644 --- a/tap.c +++ b/tap.c @@ -96,6 +96,32 @@ 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 + * + * Returns: 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 + * + * Returns: 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 +158,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 +189,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); -- 2.37.3

Callers of tap_send() can optionally use a small optimization by adding extra space for the 4 byte length header used on the qemu socket interface. tap_ip_send() is currently the only user of this, but this is used only for "slow path" ICMP and DHCP packets, so there's not a lot of value to the optimization. Worse, having the two paths here complicates the interface and makes future cleanups difficult, so just remove it. I have some plans to bring back the optimization in a more general way in future, but for now it's just in the way. Signed-off-by: David Gibson --- arp.c | 2 +- dhcp.c | 2 +- ndp.c | 2 +- tap.c | 29 +++++++++-------------------- tap.h | 2 +- 5 files changed, 13 insertions(+), 24 deletions(-) diff --git a/arp.c b/arp.c index 0ad97af..141d43f 100644 --- a/arp.c +++ b/arp.c @@ -81,7 +81,7 @@ int arp(const struct ctx *c, const struct pool *p) memcpy(eh->h_dest, eh->h_source, sizeof(eh->h_dest)); memcpy(eh->h_source, c->mac, sizeof(eh->h_source)); - if (tap_send(c, eh, len, 0) < 0) + if (tap_send(c, eh, len) < 0) perror("ARP: send"); return 1; diff --git a/dhcp.c b/dhcp.c index 875e18b..2b3af82 100644 --- a/dhcp.c +++ b/dhcp.c @@ -377,7 +377,7 @@ int dhcp(const struct ctx *c, const struct pool *p) memcpy(eh->h_dest, eh->h_source, ETH_ALEN); memcpy(eh->h_source, c->mac, ETH_ALEN); - if (tap_send(c, eh, len, 0) < 0) + if (tap_send(c, eh, len) < 0) perror("DHCP: send"); return 1; diff --git a/ndp.c b/ndp.c index 03f1d06..79be0cf 100644 --- a/ndp.c +++ b/ndp.c @@ -200,7 +200,7 @@ dns_done: memcpy(ehr->h_source, c->mac, ETH_ALEN); ehr->h_proto = htons(ETH_P_IPV6); - if (tap_send(c, ehr, len, 0) < 0) + if (tap_send(c, ehr, len) < 0) perror("NDP: send"); return 1; diff --git a/tap.c b/tap.c index 844ee43..07592dd 100644 --- a/tap.c +++ b/tap.c @@ -66,34 +66,24 @@ static PACKET_POOL_NOINIT(pool_tap6, TAP_MSGS, pkt_buf); * @c: Execution context * @data: Packet buffer * @len: Total L2 packet length - * @vnet_pre: Buffer has four-byte headroom * * Return: return code from send() or write() */ -int tap_send(const struct ctx *c, const void *data, size_t len, int vnet_pre) +int tap_send(const struct ctx *c, const void *data, size_t len) { - if (vnet_pre) - pcap((char *)data + 4, len); - else - pcap(data, len); + pcap(data, len); if (c->mode == MODE_PASST) { int flags = MSG_NOSIGNAL | MSG_DONTWAIT; + uint32_t vnet_len = htonl(len); - if (vnet_pre) { - *((uint32_t *)data) = htonl(len); - len += 4; - } else { - uint32_t vnet_len = htonl(len); - - if (send(c->fd_tap, &vnet_len, 4, flags) < 0) - return -1; - } + if (send(c->fd_tap, &vnet_len, 4, flags) < 0) + return -1; return send(c->fd_tap, data, len, flags); } - return write(c->fd_tap, (char *)data + (vnet_pre ? 4 : 0), len); + return write(c->fd_tap, (char *)data, len); } /** @@ -135,10 +125,9 @@ 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) { char buf[USHRT_MAX]; - char *pkt = buf + 4; struct ethhdr *eh; - eh = (struct ethhdr *)pkt; + eh = (struct ethhdr *)buf; /* TODO: ARP table lookup */ memcpy(eh->h_dest, c->mac_guest, ETH_ALEN); @@ -174,7 +163,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, csum_icmp4(ih, ih + 1, len - sizeof(*ih)); } - if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1) < 0) + if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh)) < 0) debug("tap: failed to send %lu bytes (IPv4)", len); } else { struct ipv6hdr *ip6h = (struct ipv6hdr *)(eh + 1); @@ -215,7 +204,7 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto, ip6h->flow_lbl[2] = (flow >> 0) & 0xff; } - if (tap_send(c, buf, len + sizeof(*ip6h) + sizeof(*eh), 1) < 1) + if (tap_send(c, buf, len + sizeof(*ip6h) + sizeof(*eh)) < 1) debug("tap: failed to send %lu bytes (IPv6)", len); } } diff --git a/tap.h b/tap.h index a6764b4..a8da8bb 100644 --- a/tap.h +++ b/tap.h @@ -11,7 +11,7 @@ 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); +int tap_send(const struct ctx *c, const void *data, size_t len); void tap_handler(struct ctx *c, int fd, uint32_t events, const struct timespec *now); void tap_sock_init(struct ctx *c); -- 2.37.3

tap_ip6_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_udp6_send() and tap_icmp6_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 IPv6 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 --- dhcpv6.c | 21 +++------------ icmp.c | 3 ++- tap.c | 82 ++++++++++++++++++++++++++++++++++++++++++-------------- tap.h | 9 +++++-- 4 files changed, 75 insertions(+), 40 deletions(-) diff --git a/dhcpv6.c b/dhcpv6.c index 7829968..e763aed 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -208,15 +208,8 @@ struct msg_hdr { uint32_t xid:24; } __attribute__((__packed__)); -#if __BYTE_ORDER == __BIG_ENDIAN -#define UH_RESP {{{ 547, 546, 0, 0, }}} -#else -#define UH_RESP {{{ __bswap_constant_16(547), __bswap_constant_16(546), 0, 0 }}} -#endif - /** * struct resp_t - Normal advertise and reply message - * @uh: UDP header * @hdr: DHCP message header * @server_id: Server Identifier option * @ia_na: Non-temporary Address option @@ -226,7 +219,6 @@ struct msg_hdr { * @dns_search: Domain Search List, here just for storage size */ static struct resp_t { - struct udphdr uh; struct msg_hdr hdr; struct opt_server_id server_id; @@ -236,7 +228,6 @@ static struct resp_t { struct opt_dns_servers dns_servers; struct opt_dns_search dns_search; } __attribute__((__packed__)) resp = { - UH_RESP, { 0 }, SERVER_ID, @@ -270,13 +261,11 @@ static const struct opt_status_code sc_not_on_link = { /** * struct resp_not_on_link_t - NotOnLink error (mandated by RFC 8415, 18.3.2.) - * @uh: UDP header * @hdr: DHCP message header * @server_id: Server Identifier option * @var: Payload: IA_NA from client, status code, client ID */ static struct resp_not_on_link_t { - struct udphdr uh; struct msg_hdr hdr; struct opt_server_id server_id; @@ -284,7 +273,6 @@ static struct resp_not_on_link_t { uint8_t var[sizeof(struct opt_ia_na) + sizeof(struct opt_status_code) + sizeof(struct opt_client_id)]; } __attribute__((__packed__)) resp_not_on_link = { - UH_RESP, { TYPE_REPLY, 0 }, SERVER_ID, { 0, }, @@ -527,12 +515,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, n += sizeof(struct opt_hdr) + ntohs(client_id->l); n = offsetof(struct resp_not_on_link_t, var) + n; - resp_not_on_link.uh.len = htons(n); resp_not_on_link.hdr.xid = mh->xid; - tap_ip6_send(c, src, IPPROTO_UDP, - (char *)&resp_not_on_link, n, mh->xid); + tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546, + mh->xid, &resp_not_on_link, n); return 1; } @@ -576,11 +563,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, n = offsetof(struct resp_t, client_id) + sizeof(struct opt_hdr) + ntohs(client_id->l); n = dhcpv6_dns_fill(c, (char *)&resp, n); - resp.uh.len = htons(n); resp.hdr.xid = mh->xid; - tap_ip6_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid); + tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546, + mh->xid, &resp, n); c->ip6.addr_seen = c->ip6.addr; return 1; diff --git a/icmp.c b/icmp.c index 61c2d90..6493ea9 100644 --- a/icmp.c +++ b/icmp.c @@ -105,7 +105,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, icmp_id_map[V6][id].seq = seq; } - tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0); + tap_icmp6_send(c, &sr6->sin6_addr, + tap_ip6_daddr(c, &sr6->sin6_addr), buf, n); } else { struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr; struct icmphdr *ih = (struct icmphdr *)buf; diff --git a/tap.c b/tap.c index 0e8c99b..135d799 100644 --- a/tap.c +++ b/tap.c @@ -175,21 +175,22 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, } /** - * tap_ip6_send() - Send IPv6 packet, with L2 headers, calculating L3/L4 checksums + * tap_push_ip6h() - Build IPv6 header for inbound packet * @c: Execution context * @src: IPv6 source address - * @proto: L4 protocol number - * @in: Payload + * @dst: IPv6 destination address * @len: L4 payload length - * @flow: Flow label + * @proto: L4 protocol number + * @flow: IPv6 flow identifier + * + * Return: pointer at which to write the packet's payload */ -void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, - uint8_t proto, const char *in, size_t len, uint32_t flow) +static void *tap_push_ip6h(char *buf, + const struct in6_addr *src, + const struct in6_addr *dst, + size_t len, uint8_t proto, uint32_t flow) { - char buf[USHRT_MAX]; - struct ipv6hdr *ip6h = - (struct ipv6hdr *)tap_push_l2h(c, buf, ETH_P_IPV6); - char *data = (char *)(ip6h + 1); + struct ipv6hdr *ip6h = (struct ipv6hdr *)buf; ip6h->payload_len = htons(len); ip6h->priority = 0; @@ -197,24 +198,65 @@ void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, ip6h->nexthdr = proto; ip6h->hop_limit = 255; ip6h->saddr = *src; - ip6h->daddr = *tap_ip6_daddr(c, src); + ip6h->daddr = *dst; ip6h->flow_lbl[0] = (flow >> 16) & 0xf; ip6h->flow_lbl[1] = (flow >> 8) & 0xff; ip6h->flow_lbl[2] = (flow >> 0) & 0xff; + return ip6h + 1; +} +/** + * tap_udp6_send() - Send UDP over IPv6 packet + * @c: Execution context + * @src: IPv6 source address + * @sport: UDP source port + * @dst: IPv6 destination address + * @dport: UDP destination port + * @flow: Flow label + * @in: UDP payload contents (not including UDP header) + * @len: UDP payload length (not including UDP header) + */ +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, + uint32_t flow, const void *in, size_t len) +{ + size_t udplen = len + sizeof(struct udphdr); + char buf[USHRT_MAX]; + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); + void *uhp = tap_push_ip6h(ip6h, src, dst, udplen, IPPROTO_UDP, flow); + struct udphdr *uh = (struct udphdr *)uhp; + char *data = (char *)(uh + 1); + + uh->source = htons(sport); + uh->dest = htons(dport); + uh->len = htons(udplen); + csum_udp6(uh, src, dst, in, len); memcpy(data, in, len); - if (proto == IPPROTO_UDP) { - struct udphdr *uh = (struct udphdr *)(ip6h + 1); + if (tap_send(c, buf, len + (data - buf)) < 1) + debug("tap: failed to send %lu bytes (IPv6)", len); +} - csum_udp6(uh, &ip6h->saddr, &ip6h->daddr, - uh + 1, len - sizeof(*uh)); - } else if (proto == IPPROTO_ICMPV6) { - struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1); +/** + * tap_icmp6_send() - Send ICMPv6 packet + * @c: Execution context + * @src: IPv6 source address + * @dst: IPv6 destination address + * @in: ICMP packet, including ICMP header + * @len: ICMP packet length, including ICMP header + */ +void tap_icmp6_send(const struct ctx *c, + const struct in6_addr *src, const struct in6_addr *dst, + void *in, size_t len) +{ + char buf[USHRT_MAX]; + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); + char *data = tap_push_ip6h(ip6h, src, dst, len, IPPROTO_ICMPV6, 0); + struct icmp6hdr *icmp6h = (struct icmp6hdr *)data; - csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr, - ih + 1, len - sizeof(*ih)); - } + memcpy(data, in, len); + csum_icmp6(icmp6h, src, dst, icmp6h + 1, len - sizeof(*icmp6h)); if (tap_send(c, buf, len + (data - buf)) < 1) debug("tap: failed to send %lu bytes (IPv6)", len); diff --git a/tap.h b/tap.h index 011ba8e..d43c7a0 100644 --- a/tap.h +++ b/tap.h @@ -11,8 +11,13 @@ 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_ip6_send(const struct ctx *c, const struct in6_addr *src, - uint8_t proto, const char *in, size_t len, uint32_t flow); +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, + uint32_t flow, const void *in, size_t len); +void tap_icmp6_send(const struct ctx *c, + const struct in6_addr *src, const struct in6_addr *dst, + void *in, size_t len); int tap_send(const struct ctx *c, const void *data, size_t len); void tap_handler(struct ctx *c, int fd, uint32_t events, const struct timespec *now); -- 2.37.3

We send ICMPv6 packets to the guest from both icmp.c and from ndp.c. The case in ndp() manually constructs L2 and IPv6 headers, unlike the version in icmp.c which uses the tap_icmp6_send() helper from tap.c Now that we've broaded the parameters of tap_icmp6_send() we can use it in ndp() as well saving some duplicated logic. Signed-off-by: David Gibson --- ndp.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/ndp.c b/ndp.c index f96b4b7..80e1f19 100644 --- a/ndp.c +++ b/ndp.c @@ -47,6 +47,7 @@ */ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr) { + const struct in6_addr *rsaddr; /* src addr for reply */ char buf[BUFSIZ] = { 0 }; struct ipv6hdr *ip6hr; struct icmp6hdr *ihr; @@ -180,26 +181,12 @@ dns_done: else c->ip6.addr_seen = *saddr; - ip6hr->daddr = *saddr; if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) - ip6hr->saddr = c->ip6.gw; + rsaddr = &c->ip6.gw; else - ip6hr->saddr = c->ip6.addr_ll; + rsaddr = &c->ip6.addr_ll; - ip6hr->payload_len = htons(sizeof(*ihr) + len); - csum_icmp6(ihr, &ip6hr->saddr, &ip6hr->daddr, ihr + 1, len); - - ip6hr->version = 6; - ip6hr->nexthdr = IPPROTO_ICMPV6; - ip6hr->hop_limit = 255; - - len += sizeof(*ehr) + sizeof(*ip6hr) + sizeof(*ihr); - memcpy(ehr->h_dest, c->mac_guest, ETH_ALEN); - memcpy(ehr->h_source, c->mac, ETH_ALEN); - ehr->h_proto = htons(ETH_P_IPV6); - - if (tap_send(c, ehr, len) < 0) - perror("NDP: send"); + tap_icmp6_send(c, rsaddr, saddr, ihr, len + sizeof(*ihr)); return 1; } -- 2.37.3

The IPv4 specific dhcp() manually constructs L2 and IP headers to send its DHCP reply packet, unlike its IPv6 equivalent in dhcpv6.c which uses the tap_udp6_send() helper. Now that we've broaded the parameters to tap_udp4_send() we can use it in dhcp() to avoid some duplicated logic. Signed-off-by: David Gibson --- dhcp.c | 18 ++---------------- tap.c | 1 - 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/dhcp.c b/dhcp.c index 2b3af82..d22698a 100644 --- a/dhcp.c +++ b/dhcp.c @@ -363,22 +363,8 @@ int dhcp(const struct ctx *c, const struct pool *p) if (!c->no_dhcp_dns_search) opt_set_dns_search(c, sizeof(m->o)); - uh->len = htons(len = offsetof(struct msg, o) + fill(m) + sizeof(*uh)); - uh->source = htons(67); - uh->dest = htons(68); - csum_udp4(uh, c->ip4.gw, c->ip4.addr, uh + 1, len - sizeof(*uh)); - - iph->tot_len = htons(len += sizeof(*iph)); - iph->daddr = c->ip4.addr; - iph->saddr = c->ip4.gw; - csum_ip4_header(iph); - - len += sizeof(*eh); - memcpy(eh->h_dest, eh->h_source, ETH_ALEN); - memcpy(eh->h_source, c->mac, ETH_ALEN); - - if (tap_send(c, eh, len) < 0) - perror("DHCP: send"); + len = offsetof(struct msg, o) + fill(m); + tap_udp4_send(c, c->ip4.gw, 67, c->ip4.addr, 68, m, len); return 1; } diff --git a/tap.c b/tap.c index d250a0b..3f78c99 100644 --- a/tap.c +++ b/tap.c @@ -170,7 +170,6 @@ static void *tap_push_ip4h(char *buf, in_addr_t src, in_addr_t dst, * @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) -- 2.37.3