* [PATCH v4 0/8] Add vhost-user support to passt (part 1)
@ 2024-02-29 16:59 Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
` (7 more replies)
0 siblings, 8 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier
v4:
- rebase
- see detailed v4 history log in each patch
v3:
- add a patch that has been extracted from:
"tcp: extract buffer management from tcp_send_flag()"
-> "tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()"
- see detailed v3 history log in each patch
- I didn't address the alignment problem when we provide a pointer
to a sub-structure in the internal buffer structure.
(for the last patches of the series).
v2 comparing to vhost-user full part:
- part 1 includes only preliminary patches (checksum, iovec, cleanup)
- see detailed v2 history log in each patch.
Full series v1 available at:
[PATCH 00/24] Add vhost-user support to passt.
https://url.corp.redhat.com/passt-vhost-user-v1
Thanks,
Laurent
Laurent Vivier (8):
pcap: add pcap_iov()
checksum: align buffers
checksum: add csum_iov()
util: move IP stuff from util.[ch] to ip.[ch]
checksum: use csum_ip4_header() in udp.c and tcp.c
checksum: introduce functions to compute the header part checksum for
TCP/UDP
tap: make tap_update_mac() generic
tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()
Makefile | 10 +--
checksum.c | 173 ++++++++++++++++++++++++++++++-----------
checksum.h | 9 ++-
conf.c | 1 +
dhcp.c | 1 +
flow.c | 1 +
fwd.c | 1 +
icmp.c | 1 +
inany.c | 1 +
ip.c | 72 +++++++++++++++++
ip.h | 86 +++++++++++++++++++++
ndp.c | 1 +
pcap.c | 26 ++++++-
pcap.h | 1 +
qrap.c | 1 +
tap.c | 14 ++--
tap.h | 2 +-
tcp.c | 214 +++++++++++++++++++++++++++++----------------------
tcp_splice.c | 1 +
udp.c | 38 ++++-----
util.c | 55 -------------
util.h | 76 ------------------
22 files changed, 475 insertions(+), 310 deletions(-)
create mode 100644 ip.c
create mode 100644 ip.h
--
2.42.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/8] pcap: add pcap_iov()
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 23:16 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 2/8] checksum: align buffers Laurent Vivier
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier
Introduce a new function pcap_iov() to capture packet desribed by an IO
vector.
Update pcap_frame() to manage iovcnt > 1.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v4:
- use pcap_frame()
v3:
- update rationale
- update comment
- use strerror(errno)
- use size_t for io vector length
v2:
- introduce pcap_header(), a common helper to write
packet header
- use writev() rather than write() in a loop
- add functions comment
pcap.c | 26 ++++++++++++++++++++++----
pcap.h | 1 +
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/pcap.c b/pcap.c
index a4057b5f9c6a..372f02045262 100644
--- a/pcap.c
+++ b/pcap.c
@@ -32,6 +32,7 @@
#include "passt.h"
#include "log.h"
#include "pcap.h"
+#include "iov.h"
#define PCAP_VERSION_MINOR 4
@@ -78,7 +79,7 @@ struct pcap_pkthdr {
static void pcap_frame(const struct iovec *iov, size_t iovcnt,
size_t offset, const struct timeval *tv)
{
- size_t len = iov->iov_len - offset;
+ size_t len = iov_size(iov, iovcnt) - offset;
struct pcap_pkthdr h = {
.tv_sec = tv->tv_sec,
.tv_usec = tv->tv_usec,
@@ -87,10 +88,8 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
};
struct iovec hiov = { &h, sizeof(h) };
- (void)iovcnt;
-
if (write_remainder(pcap_fd, &hiov, 1, 0) < 0 ||
- write_remainder(pcap_fd, iov, 1, offset) < 0) {
+ write_remainder(pcap_fd, iov, iovcnt, offset) < 0) {
debug("Cannot log packet, length %zu: %s",
len, strerror(errno));
}
@@ -135,6 +134,25 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
pcap_frame(iov + i * frame_parts, frame_parts, offset, &tv);
}
+/*
+ * pcap_iov - Write packet data described by an I/O vector
+ * to a pcap file descriptor.
+ *
+ * @iov: Pointer to the array of struct iovec describing the I/O vector
+ * containing packet data to write, including L2 header
+ * @iovcnt: Number of buffers (@iov entries)
+ */
+void pcap_iov(const struct iovec *iov, size_t iovcnt)
+{
+ struct timeval tv;
+
+ if (pcap_fd == -1)
+ return;
+
+ gettimeofday(&tv, NULL);
+ pcap_frame(iov, iovcnt, 0, &tv);
+}
+
/**
* pcap_init() - Initialise pcap file
* @c: Execution context
diff --git a/pcap.h b/pcap.h
index 85fc58e57572..b1c4c909c109 100644
--- a/pcap.h
+++ b/pcap.h
@@ -9,6 +9,7 @@
void pcap(const char *pkt, size_t len);
void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
size_t offset);
+void pcap_iov(const struct iovec *iov, size_t n);
void pcap_init(struct ctx *c);
#endif /* PCAP_H */
--
@@ -9,6 +9,7 @@
void pcap(const char *pkt, size_t len);
void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
size_t offset);
+void pcap_iov(const struct iovec *iov, size_t n);
void pcap_init(struct ctx *c);
#endif /* PCAP_H */
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/8] checksum: align buffers
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 3/8] checksum: add csum_iov() Laurent Vivier
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier, David Gibson
If buffer is not aligned use sum_16b() only on the not aligned
part, and then use csum_avx2() on the remaining part
Remove unneeded now function csum_unaligned().
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
Notes:
v4:
- rebase
v3:
- Add David's R-b
v2:
- use ROUND_UP() and sizeof(__m256i)
- fix function comment
- remove csum_unaligned() and use csum() instead
checksum.c | 47 ++++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 23 deletions(-)
diff --git a/checksum.c b/checksum.c
index f21c9b7a14d1..65486b4625ba 100644
--- a/checksum.c
+++ b/checksum.c
@@ -56,6 +56,8 @@
#include <linux/udp.h>
#include <linux/icmpv6.h>
+#include "util.h"
+
/* 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
@@ -110,20 +112,7 @@ uint16_t csum_fold(uint32_t sum)
return sum;
}
-/**
- * csum_unaligned() - Compute TCP/IP-style checksum for not 32-byte aligned data
- * @buf: Input data
- * @len: Input length
- * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
- *
- * Return: 16-bit IPv4-style checksum
- */
-/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */
-__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
-uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
-{
- return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
-}
+uint16_t csum(const void *buf, size_t len, uint32_t init);
/**
* csum_ip4_header() - Calculate and set IPv4 header checksum
@@ -132,7 +121,7 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
void csum_ip4_header(struct iphdr *ip4h)
{
ip4h->check = 0;
- ip4h->check = csum_unaligned(ip4h, (size_t)ip4h->ihl * 4, 0);
+ ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
}
/**
@@ -159,7 +148,7 @@ void csum_udp4(struct udphdr *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);
+ udp4hr->check = csum(payload, len, psum);
}
}
@@ -178,7 +167,7 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)
/* Partial checksum for ICMP header alone */
psum = sum_16b(icmp4hr, sizeof(*icmp4hr));
- icmp4hr->checksum = csum_unaligned(payload, len, psum);
+ icmp4hr->checksum = csum(payload, len, psum);
}
/**
@@ -199,7 +188,7 @@ void csum_udp6(struct udphdr *udp6hr,
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);
+ udp6hr->check = csum(payload, len, psum);
}
/**
@@ -222,7 +211,7 @@ void csum_icmp6(struct icmp6hdr *icmp6hr,
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);
+ icmp6hr->icmp6_cksum = csum(payload, len, psum);
}
#ifdef __AVX2__
@@ -397,17 +386,29 @@ less_than_128_bytes:
/**
* csum() - Compute TCP/IP-style checksum
- * @buf: Input buffer, must be aligned to 32-byte boundary
+ * @buf: Input buffer
* @len: Input length
* @init: Initial 32-bit checksum, 0 for no pre-computed checksum
*
- * Return: 16-bit folded, complemented checksum sum
+ * Return: 16-bit folded, complemented checksum
*/
/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
uint16_t csum(const void *buf, size_t len, uint32_t init)
{
- return (uint16_t)~csum_fold(csum_avx2(buf, len, init));
+ intptr_t align = ROUND_UP((intptr_t)buf, sizeof(__m256i));
+ unsigned int pad = align - (intptr_t)buf;
+
+ if (len < pad)
+ pad = len;
+
+ if (pad)
+ init += sum_16b(buf, pad);
+
+ if (len > pad)
+ init = csum_avx2((void *)align, len - pad, init);
+
+ return (uint16_t)~csum_fold(init);
}
#else /* __AVX2__ */
@@ -424,7 +425,7 @@ uint16_t csum(const void *buf, size_t len, uint32_t init)
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
uint16_t csum(const void *buf, size_t len, uint32_t init)
{
- return csum_unaligned(buf, len, init);
+ return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
}
#endif /* !__AVX2__ */
--
@@ -56,6 +56,8 @@
#include <linux/udp.h>
#include <linux/icmpv6.h>
+#include "util.h"
+
/* 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
@@ -110,20 +112,7 @@ uint16_t csum_fold(uint32_t sum)
return sum;
}
-/**
- * csum_unaligned() - Compute TCP/IP-style checksum for not 32-byte aligned data
- * @buf: Input data
- * @len: Input length
- * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
- *
- * Return: 16-bit IPv4-style checksum
- */
-/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */
-__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
-uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
-{
- return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
-}
+uint16_t csum(const void *buf, size_t len, uint32_t init);
/**
* csum_ip4_header() - Calculate and set IPv4 header checksum
@@ -132,7 +121,7 @@ uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init)
void csum_ip4_header(struct iphdr *ip4h)
{
ip4h->check = 0;
- ip4h->check = csum_unaligned(ip4h, (size_t)ip4h->ihl * 4, 0);
+ ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
}
/**
@@ -159,7 +148,7 @@ void csum_udp4(struct udphdr *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);
+ udp4hr->check = csum(payload, len, psum);
}
}
@@ -178,7 +167,7 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)
/* Partial checksum for ICMP header alone */
psum = sum_16b(icmp4hr, sizeof(*icmp4hr));
- icmp4hr->checksum = csum_unaligned(payload, len, psum);
+ icmp4hr->checksum = csum(payload, len, psum);
}
/**
@@ -199,7 +188,7 @@ void csum_udp6(struct udphdr *udp6hr,
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);
+ udp6hr->check = csum(payload, len, psum);
}
/**
@@ -222,7 +211,7 @@ void csum_icmp6(struct icmp6hdr *icmp6hr,
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);
+ icmp6hr->icmp6_cksum = csum(payload, len, psum);
}
#ifdef __AVX2__
@@ -397,17 +386,29 @@ less_than_128_bytes:
/**
* csum() - Compute TCP/IP-style checksum
- * @buf: Input buffer, must be aligned to 32-byte boundary
+ * @buf: Input buffer
* @len: Input length
* @init: Initial 32-bit checksum, 0 for no pre-computed checksum
*
- * Return: 16-bit folded, complemented checksum sum
+ * Return: 16-bit folded, complemented checksum
*/
/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
uint16_t csum(const void *buf, size_t len, uint32_t init)
{
- return (uint16_t)~csum_fold(csum_avx2(buf, len, init));
+ intptr_t align = ROUND_UP((intptr_t)buf, sizeof(__m256i));
+ unsigned int pad = align - (intptr_t)buf;
+
+ if (len < pad)
+ pad = len;
+
+ if (pad)
+ init += sum_16b(buf, pad);
+
+ if (len > pad)
+ init = csum_avx2((void *)align, len - pad, init);
+
+ return (uint16_t)~csum_fold(init);
}
#else /* __AVX2__ */
@@ -424,7 +425,7 @@ uint16_t csum(const void *buf, size_t len, uint32_t init)
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
uint16_t csum(const void *buf, size_t len, uint32_t init)
{
- return csum_unaligned(buf, len, init);
+ return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
}
#endif /* !__AVX2__ */
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 3/8] checksum: add csum_iov()
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 2/8] checksum: align buffers Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
` (4 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier, David Gibson
Introduce the function csum_unfolded() that computes the unfolded
32-bit checksum of a data buffer, and call it from csum() that returns
the folded value.
Introduce csum_iov() that computes the checksum using csum_folded() on
all vectors of the iovec array and returns the folded result.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
Notes:
v4:
- rebase
v3:
- update comments
- use size_t for the IO vectors length
- include checksum.h in checksum.c
- export csum_unfolded() (for later)
v2:
- fix typo and superfluous space
- update comments
checksum.c | 56 ++++++++++++++++++++++++++++++++++++++++++------------
checksum.h | 2 ++
2 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/checksum.c b/checksum.c
index 65486b4625ba..74e3742bc6f6 100644
--- a/checksum.c
+++ b/checksum.c
@@ -57,6 +57,7 @@
#include <linux/icmpv6.h>
#include "util.h"
+#include "checksum.h"
/* 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
@@ -385,16 +386,16 @@ less_than_128_bytes:
}
/**
- * csum() - Compute TCP/IP-style checksum
- * @buf: Input buffer
- * @len: Input length
- * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
+ * csum_unfolded - Calculate the unfolded checksum of a data buffer.
*
- * Return: 16-bit folded, complemented checksum
+ * @buf: Input buffer
+ * @len: Input length
+ * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
+ *
+ * Return: 32-bit unfolded
*/
-/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
-uint16_t csum(const void *buf, size_t len, uint32_t init)
+uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init)
{
intptr_t align = ROUND_UP((intptr_t)buf, sizeof(__m256i));
unsigned int pad = align - (intptr_t)buf;
@@ -408,16 +409,30 @@ uint16_t csum(const void *buf, size_t len, uint32_t init)
if (len > pad)
init = csum_avx2((void *)align, len - pad, init);
- return (uint16_t)~csum_fold(init);
+ return init;
}
-
#else /* __AVX2__ */
+/**
+ * csum_unfolded - Calculate the unfolded checksum of a data buffer.
+ *
+ * @buf: Input buffer
+ * @len: Input length
+ * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
+ *
+ * Return: 32-bit unfolded checksum
+ */
+__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
+uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init)
+{
+ return sum_16b(buf, len) + init;
+}
+#endif /* !__AVX2__ */
/**
* csum() - Compute TCP/IP-style checksum
* @buf: Input buffer
* @len: Input length
- * @sum: Initial 32-bit checksum, 0 for no pre-computed checksum
+ * @init: Initial 32-bit checksum, 0 for no pre-computed checksum
*
* Return: 16-bit folded, complemented checksum
*/
@@ -425,7 +440,24 @@ uint16_t csum(const void *buf, size_t len, uint32_t init)
__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */
uint16_t csum(const void *buf, size_t len, uint32_t init)
{
- return (uint16_t)~csum_fold(sum_16b(buf, len) + init);
+ return (uint16_t)~csum_fold(csum_unfolded(buf, len, init));
}
-#endif /* !__AVX2__ */
+/**
+ * csum_iov() - Calculates the unfolded checksum over an array of IO vectors
+ *
+ * @iov Pointer to the array of IO vectors
+ * @n Length of the array
+ * @init Initial 32-bit checksum, 0 for no pre-computed checksum
+ *
+ * Return: 16-bit folded, complemented checksum
+ */
+uint16_t csum_iov(struct iovec *iov, size_t n, uint32_t init)
+{
+ unsigned int i;
+
+ for (i = 0; i < n; i++)
+ init = csum_unfolded(iov[i].iov_base, iov[i].iov_len, init);
+
+ return (uint16_t)~csum_fold(init);
+}
diff --git a/checksum.h b/checksum.h
index 21c0310d3804..dfa705a04a24 100644
--- a/checksum.h
+++ b/checksum.h
@@ -24,6 +24,8 @@ void csum_udp6(struct udphdr *udp6hr,
void csum_icmp6(struct icmp6hdr *icmp6hr,
const struct in6_addr *saddr, const struct in6_addr *daddr,
const void *payload, size_t len);
+uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init);
uint16_t csum(const void *buf, size_t len, uint32_t init);
+uint16_t csum_iov(struct iovec *iov, size_t n, uint32_t init);
#endif /* CHECKSUM_H */
--
@@ -24,6 +24,8 @@ void csum_udp6(struct udphdr *udp6hr,
void csum_icmp6(struct icmp6hdr *icmp6hr,
const struct in6_addr *saddr, const struct in6_addr *daddr,
const void *payload, size_t len);
+uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init);
uint16_t csum(const void *buf, size_t len, uint32_t init);
+uint16_t csum_iov(struct iovec *iov, size_t n, uint32_t init);
#endif /* CHECKSUM_H */
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch]
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
` (2 preceding siblings ...)
2024-02-29 16:59 ` [PATCH v4 3/8] checksum: add csum_iov() Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 23:19 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier, David Gibson
Introduce ip.[ch] file to encapsulate IP protocol handling functions and
structures. Modify various files to include the new header ip.h when
it's needed.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
Notes:
v4:
- rebase
v3:
- rewrap rationale
- add David's R-b
v2:
- update rationale and comments
Makefile | 10 +++---
conf.c | 1 +
dhcp.c | 1 +
flow.c | 1 +
fwd.c | 1 +
icmp.c | 1 +
inany.c | 1 +
ip.c | 72 +++++++++++++++++++++++++++++++++++++++++++
ip.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
ndp.c | 1 +
qrap.c | 1 +
tap.c | 1 +
tcp.c | 1 +
tcp_splice.c | 1 +
udp.c | 1 +
util.c | 55 ---------------------------------
util.h | 76 ----------------------------------------------
17 files changed, 175 insertions(+), 136 deletions(-)
create mode 100644 ip.c
create mode 100644 ip.h
diff --git a/Makefile b/Makefile
index 8f9669413530..2735797a054c 100644
--- a/Makefile
+++ b/Makefile
@@ -45,8 +45,8 @@ FLAGS += -DVERSION=\"$(VERSION)\"
FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \
- icmp.c igmp.c inany.c iov.c isolation.c lineread.c log.c mld.c ndp.c \
- netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \
+ icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \
+ ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \
tcp_splice.c udp.c util.c
QRAP_SRCS = qrap.c
SRCS = $(PASST_SRCS) $(QRAP_SRCS)
@@ -54,9 +54,9 @@ SRCS = $(PASST_SRCS) $(QRAP_SRCS)
MANPAGES = passt.1 pasta.1 qrap.1
PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \
- flow_table.h icmp.h inany.h iov.h isolation.h lineread.h log.h ndp.h \
- netlink.h packet.h passt.h pasta.h pcap.h pif.h siphash.h tap.h tcp.h \
- tcp_conn.h tcp_splice.h udp.h util.h
+ flow_table.h icmp.h inany.h iov.h ip.h isolation.h lineread.h log.h \
+ ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h siphash.h tap.h \
+ tcp.h tcp_conn.h tcp_splice.h udp.h util.h
HEADERS = $(PASST_HEADERS) seccomp.h
C := \#include <linux/tcp.h>\nstruct tcp_info x = { .tcpi_snd_wnd = 0 };
diff --git a/conf.c b/conf.c
index e630140def5b..4a783b8b1410 100644
--- a/conf.c
+++ b/conf.c
@@ -35,6 +35,7 @@
#include <netinet/if_ether.h>
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "netlink.h"
#include "udp.h"
diff --git a/dhcp.c b/dhcp.c
index 110772867632..ff4834a3dce9 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -25,6 +25,7 @@
#include <limits.h>
#include "util.h"
+#include "ip.h"
#include "checksum.h"
#include "packet.h"
#include "passt.h"
diff --git a/flow.c b/flow.c
index d7974d59974c..5bb24ccf1504 100644
--- a/flow.c
+++ b/flow.c
@@ -11,6 +11,7 @@
#include <string.h>
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "siphash.h"
#include "inany.h"
diff --git a/fwd.c b/fwd.c
index 09650b26db11..a235d1315cd6 100644
--- a/fwd.c
+++ b/fwd.c
@@ -21,6 +21,7 @@
#include <stdio.h>
#include "util.h"
+#include "ip.h"
#include "fwd.h"
#include "passt.h"
#include "lineread.h"
diff --git a/icmp.c b/icmp.c
index fb2fcafc8b86..49d6dd922212 100644
--- a/icmp.c
+++ b/icmp.c
@@ -33,6 +33,7 @@
#include "packet.h"
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "tap.h"
#include "log.h"
diff --git a/inany.c b/inany.c
index 1c165b144817..c8479a755818 100644
--- a/inany.c
+++ b/inany.c
@@ -13,6 +13,7 @@
#include <arpa/inet.h>
#include "util.h"
+#include "ip.h"
#include "siphash.h"
#include "inany.h"
diff --git a/ip.c b/ip.c
new file mode 100644
index 000000000000..2cc7f6548aff
--- /dev/null
+++ b/ip.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* PASST - Plug A Simple Socket Transport
+ * for qemu/UNIX domain socket mode
+ *
+ * PASTA - Pack A Subtle Tap Abstraction
+ * for network namespace/tap device mode
+ *
+ * ip.c - IP related functions
+ *
+ * Copyright (c) 2020-2021 Red Hat GmbH
+ * Author: Stefano Brivio <sbrivio@redhat.com>
+ */
+
+#include <stddef.h>
+#include "util.h"
+#include "ip.h"
+
+#define IPV6_NH_OPT(nh) \
+ ((nh) == 0 || (nh) == 43 || (nh) == 44 || (nh) == 50 || \
+ (nh) == 51 || (nh) == 60 || (nh) == 135 || (nh) == 139 || \
+ (nh) == 140 || (nh) == 253 || (nh) == 254)
+
+/**
+ * ipv6_l4hdr() - Find pointer to L4 header in IPv6 packet and extract protocol
+ * @p: Packet pool, packet number @idx has IPv6 header at @offset
+ * @idx: Index of packet in pool
+ * @offset: Pre-calculated IPv6 header offset
+ * @proto: Filled with L4 protocol number
+ * @dlen: Data length (payload excluding header extensions), set on return
+ *
+ * Return: pointer to L4 header, NULL if not found
+ */
+char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
+ size_t *dlen)
+{
+ const struct ipv6_opt_hdr *o;
+ const struct ipv6hdr *ip6h;
+ char *base;
+ int hdrlen;
+ uint8_t nh;
+
+ base = packet_get(p, idx, 0, 0, NULL);
+ ip6h = packet_get(p, idx, offset, sizeof(*ip6h), dlen);
+ if (!ip6h)
+ return NULL;
+
+ offset += sizeof(*ip6h);
+
+ nh = ip6h->nexthdr;
+ if (!IPV6_NH_OPT(nh))
+ goto found;
+
+ while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
+ nh = o->nexthdr;
+ hdrlen = (o->hdrlen + 1) * 8;
+
+ if (IPV6_NH_OPT(nh))
+ offset += hdrlen;
+ else
+ goto found;
+ }
+
+ return NULL;
+
+found:
+ if (nh == 59)
+ return NULL;
+
+ *proto = nh;
+ return base + offset;
+}
diff --git a/ip.h b/ip.h
new file mode 100644
index 000000000000..9be47783a11e
--- /dev/null
+++ b/ip.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (c) 2021 Red Hat GmbH
+ * Author: Stefano Brivio <sbrivio@redhat.com>
+ */
+
+#ifndef IP_H
+#define IP_H
+
+#include <netinet/ip.h>
+#include <netinet/ip6.h>
+
+#define IN4_IS_ADDR_UNSPECIFIED(a) \
+ (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_ANY))
+#define IN4_IS_ADDR_BROADCAST(a) \
+ (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_BROADCAST))
+#define IN4_IS_ADDR_LOOPBACK(a) \
+ (ntohl(((struct in_addr *)(a))->s_addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET)
+#define IN4_IS_ADDR_MULTICAST(a) \
+ (IN_MULTICAST(ntohl(((struct in_addr *)(a))->s_addr)))
+#define IN4_ARE_ADDR_EQUAL(a, b) \
+ (((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
+#define IN4ADDR_LOOPBACK_INIT \
+ { .s_addr = htonl_constant(INADDR_LOOPBACK) }
+#define IN4ADDR_ANY_INIT \
+ { .s_addr = htonl_constant(INADDR_ANY) }
+
+#define L2_BUF_IP4_INIT(proto) \
+ { \
+ .version = 4, \
+ .ihl = 5, \
+ .tos = 0, \
+ .tot_len = 0, \
+ .id = 0, \
+ .frag_off = 0, \
+ .ttl = 0xff, \
+ .protocol = (proto), \
+ .saddr = 0, \
+ .daddr = 0, \
+ }
+#define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + \
+ (uint32_t)htons_constant(0xff00 | (proto)))
+
+#define L2_BUF_IP6_INIT(proto) \
+ { \
+ .priority = 0, \
+ .version = 6, \
+ .flow_lbl = { 0 }, \
+ .payload_len = 0, \
+ .nexthdr = (proto), \
+ .hop_limit = 255, \
+ .saddr = IN6ADDR_ANY_INIT, \
+ .daddr = IN6ADDR_ANY_INIT, \
+ }
+
+struct ipv6hdr {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wpedantic"
+#if __BYTE_ORDER == __BIG_ENDIAN
+ uint8_t version:4,
+ priority:4;
+#else
+ uint8_t priority:4,
+ version:4;
+#endif
+#pragma GCC diagnostic pop
+ uint8_t flow_lbl[3];
+
+ uint16_t payload_len;
+ uint8_t nexthdr;
+ uint8_t hop_limit;
+
+ struct in6_addr saddr;
+ struct in6_addr daddr;
+};
+
+struct ipv6_opt_hdr {
+ uint8_t nexthdr;
+ uint8_t hdrlen;
+ /*
+ * TLV encoded option data follows.
+ */
+} __attribute__((packed)); /* required for some archs */
+
+char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
+ size_t *dlen);
+#endif /* IP_H */
diff --git a/ndp.c b/ndp.c
index 4c85ab8bcaee..c58f4b222b76 100644
--- a/ndp.c
+++ b/ndp.c
@@ -28,6 +28,7 @@
#include "checksum.h"
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "tap.h"
#include "log.h"
diff --git a/qrap.c b/qrap.c
index 97f350a4bf0b..d59670621731 100644
--- a/qrap.c
+++ b/qrap.c
@@ -32,6 +32,7 @@
#include <linux/icmpv6.h>
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "arp.h"
diff --git a/tap.c b/tap.c
index 3a666212923e..d35d8944fc41 100644
--- a/tap.c
+++ b/tap.c
@@ -45,6 +45,7 @@
#include "checksum.h"
#include "util.h"
+#include "ip.h"
#include "iov.h"
#include "passt.h"
#include "arp.h"
diff --git a/tcp.c b/tcp.c
index 560d1d49def1..e0588f92e65f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -290,6 +290,7 @@
#include "checksum.h"
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "tap.h"
#include "siphash.h"
diff --git a/tcp_splice.c b/tcp_splice.c
index 4957abb81cde..d066112cd645 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -49,6 +49,7 @@
#include <sys/socket.h>
#include "util.h"
+#include "ip.h"
#include "passt.h"
#include "log.h"
#include "tcp_splice.h"
diff --git a/udp.c b/udp.c
index cb7c31f74403..26774df7018c 100644
--- a/udp.c
+++ b/udp.c
@@ -113,6 +113,7 @@
#include "checksum.h"
#include "util.h"
+#include "ip.h"
#include "siphash.h"
#include "inany.h"
#include "passt.h"
diff --git a/util.c b/util.c
index 81449b789b62..bac5a53489f2 100644
--- a/util.c
+++ b/util.c
@@ -32,61 +32,6 @@
#include "packet.h"
#include "log.h"
-#define IPV6_NH_OPT(nh) \
- ((nh) == 0 || (nh) == 43 || (nh) == 44 || (nh) == 50 || \
- (nh) == 51 || (nh) == 60 || (nh) == 135 || (nh) == 139 || \
- (nh) == 140 || (nh) == 253 || (nh) == 254)
-
-/**
- * ipv6_l4hdr() - Find pointer to L4 header in IPv6 packet and extract protocol
- * @p: Packet pool, packet number @idx has IPv6 header at @offset
- * @idx: Index of packet in pool
- * @offset: Pre-calculated IPv6 header offset
- * @proto: Filled with L4 protocol number
- * @dlen: Data length (payload excluding header extensions), set on return
- *
- * Return: pointer to L4 header, NULL if not found
- */
-char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
- size_t *dlen)
-{
- const struct ipv6_opt_hdr *o;
- const struct ipv6hdr *ip6h;
- char *base;
- int hdrlen;
- uint8_t nh;
-
- base = packet_get(p, idx, 0, 0, NULL);
- ip6h = packet_get(p, idx, offset, sizeof(*ip6h), dlen);
- if (!ip6h)
- return NULL;
-
- offset += sizeof(*ip6h);
-
- nh = ip6h->nexthdr;
- if (!IPV6_NH_OPT(nh))
- goto found;
-
- while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
- nh = o->nexthdr;
- hdrlen = (o->hdrlen + 1) * 8;
-
- if (IPV6_NH_OPT(nh))
- offset += hdrlen;
- else
- goto found;
- }
-
- return NULL;
-
-found:
- if (nh == 59)
- return NULL;
-
- *proto = nh;
- return base + offset;
-}
-
/**
* sock_l4() - Create and bind socket for given L4, add to epoll list
* @c: Execution context
diff --git a/util.h b/util.h
index 55513490890a..25e54a778478 100644
--- a/util.h
+++ b/util.h
@@ -110,22 +110,6 @@
#define htonl_constant(x) (__bswap_constant_32(x))
#endif
-#define IN4_IS_ADDR_UNSPECIFIED(a) \
- (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_ANY))
-#define IN4_IS_ADDR_BROADCAST(a) \
- (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_BROADCAST))
-#define IN4_IS_ADDR_LOOPBACK(a) \
- (ntohl(((struct in_addr *)(a))->s_addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET)
-#define IN4_IS_ADDR_MULTICAST(a) \
- (IN_MULTICAST(ntohl(((struct in_addr *)(a))->s_addr)))
-#define IN4_ARE_ADDR_EQUAL(a, b) \
- (((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
-#define IN4ADDR_LOOPBACK_INIT \
- { .s_addr = htonl_constant(INADDR_LOOPBACK) }
-#define IN4ADDR_ANY_INIT \
- { .s_addr = htonl_constant(INADDR_ANY) }
-
-
#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 8)
int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
void *arg);
@@ -138,34 +122,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
(void *)(arg)); \
} while (0)
-#define L2_BUF_IP4_INIT(proto) \
- { \
- .version = 4, \
- .ihl = 5, \
- .tos = 0, \
- .tot_len = 0, \
- .id = 0, \
- .frag_off = 0, \
- .ttl = 0xff, \
- .protocol = (proto), \
- .saddr = 0, \
- .daddr = 0, \
- }
-#define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + \
- (uint32_t)htons_constant(0xff00 | (proto)))
-
-#define L2_BUF_IP6_INIT(proto) \
- { \
- .priority = 0, \
- .version = 6, \
- .flow_lbl = { 0 }, \
- .payload_len = 0, \
- .nexthdr = (proto), \
- .hop_limit = 255, \
- .saddr = IN6ADDR_ANY_INIT, \
- .daddr = IN6ADDR_ANY_INIT, \
- }
-
#define RCVBUF_BIG (2UL * 1024 * 1024)
#define SNDBUF_BIG (4UL * 1024 * 1024)
#define SNDBUF_SMALL (128UL * 1024)
@@ -173,45 +129,13 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
#include <net/if.h>
#include <limits.h>
#include <stdint.h>
-#include <netinet/ip6.h>
#include "packet.h"
struct ctx;
-struct ipv6hdr {
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpedantic"
-#if __BYTE_ORDER == __BIG_ENDIAN
- uint8_t version:4,
- priority:4;
-#else
- uint8_t priority:4,
- version:4;
-#endif
-#pragma GCC diagnostic pop
- uint8_t flow_lbl[3];
-
- uint16_t payload_len;
- uint8_t nexthdr;
- uint8_t hop_limit;
-
- struct in6_addr saddr;
- struct in6_addr daddr;
-};
-
-struct ipv6_opt_hdr {
- uint8_t nexthdr;
- uint8_t hdrlen;
- /*
- * TLV encoded option data follows.
- */
-} __attribute__((packed)); /* required for some archs */
-
/* cppcheck-suppress funcArgNamesDifferent */
__attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
-char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
- size_t *dlen);
int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
const void *bind_addr, const char *ifname, uint16_t port,
uint32_t data);
--
@@ -110,22 +110,6 @@
#define htonl_constant(x) (__bswap_constant_32(x))
#endif
-#define IN4_IS_ADDR_UNSPECIFIED(a) \
- (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_ANY))
-#define IN4_IS_ADDR_BROADCAST(a) \
- (((struct in_addr *)(a))->s_addr == htonl_constant(INADDR_BROADCAST))
-#define IN4_IS_ADDR_LOOPBACK(a) \
- (ntohl(((struct in_addr *)(a))->s_addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET)
-#define IN4_IS_ADDR_MULTICAST(a) \
- (IN_MULTICAST(ntohl(((struct in_addr *)(a))->s_addr)))
-#define IN4_ARE_ADDR_EQUAL(a, b) \
- (((struct in_addr *)(a))->s_addr == ((struct in_addr *)b)->s_addr)
-#define IN4ADDR_LOOPBACK_INIT \
- { .s_addr = htonl_constant(INADDR_LOOPBACK) }
-#define IN4ADDR_ANY_INIT \
- { .s_addr = htonl_constant(INADDR_ANY) }
-
-
#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 8)
int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
void *arg);
@@ -138,34 +122,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
(void *)(arg)); \
} while (0)
-#define L2_BUF_IP4_INIT(proto) \
- { \
- .version = 4, \
- .ihl = 5, \
- .tos = 0, \
- .tot_len = 0, \
- .id = 0, \
- .frag_off = 0, \
- .ttl = 0xff, \
- .protocol = (proto), \
- .saddr = 0, \
- .daddr = 0, \
- }
-#define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + \
- (uint32_t)htons_constant(0xff00 | (proto)))
-
-#define L2_BUF_IP6_INIT(proto) \
- { \
- .priority = 0, \
- .version = 6, \
- .flow_lbl = { 0 }, \
- .payload_len = 0, \
- .nexthdr = (proto), \
- .hop_limit = 255, \
- .saddr = IN6ADDR_ANY_INIT, \
- .daddr = IN6ADDR_ANY_INIT, \
- }
-
#define RCVBUF_BIG (2UL * 1024 * 1024)
#define SNDBUF_BIG (4UL * 1024 * 1024)
#define SNDBUF_SMALL (128UL * 1024)
@@ -173,45 +129,13 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
#include <net/if.h>
#include <limits.h>
#include <stdint.h>
-#include <netinet/ip6.h>
#include "packet.h"
struct ctx;
-struct ipv6hdr {
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wpedantic"
-#if __BYTE_ORDER == __BIG_ENDIAN
- uint8_t version:4,
- priority:4;
-#else
- uint8_t priority:4,
- version:4;
-#endif
-#pragma GCC diagnostic pop
- uint8_t flow_lbl[3];
-
- uint16_t payload_len;
- uint8_t nexthdr;
- uint8_t hop_limit;
-
- struct in6_addr saddr;
- struct in6_addr daddr;
-};
-
-struct ipv6_opt_hdr {
- uint8_t nexthdr;
- uint8_t hdrlen;
- /*
- * TLV encoded option data follows.
- */
-} __attribute__((packed)); /* required for some archs */
-
/* cppcheck-suppress funcArgNamesDifferent */
__attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
-char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
- size_t *dlen);
int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
const void *bind_addr, const char *ifname, uint16_t port,
uint32_t data);
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
` (3 preceding siblings ...)
2024-02-29 16:59 ` [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 23:25 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier, David Gibson
We can find the same function to compute the IPv4 header
checksum in tcp.c, udp.c and tap.c
Use the function defined for tap.c, csum_ip4_header(), but
with the code used in tcp.c and udp.c as it doesn't need a fully
initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
Notes:
v4:
- rebase
v3:
- function parameters provide tot_len, saddr, daddr and protocol
rather than an iphdr
v2:
- use csum_ip4_header() from checksum.c
- use code from tcp.c and udp.c in csum_ip4_header()
- use "const struct iphfr *", check is not updated by the
function but by the caller.
checksum.c | 17 +++++++++++++----
checksum.h | 3 ++-
tap.c | 3 ++-
tcp.c | 24 +++---------------------
udp.c | 20 ++------------------
5 files changed, 22 insertions(+), 45 deletions(-)
diff --git a/checksum.c b/checksum.c
index 74e3742bc6f6..511b296a9a80 100644
--- a/checksum.c
+++ b/checksum.c
@@ -57,6 +57,7 @@
#include <linux/icmpv6.h>
#include "util.h"
+#include "ip.h"
#include "checksum.h"
/* Checksums are optional for UDP over IPv4, so we usually just set
@@ -116,13 +117,21 @@ uint16_t csum_fold(uint32_t sum)
uint16_t csum(const void *buf, size_t len, uint32_t init);
/**
- * csum_ip4_header() - Calculate and set IPv4 header checksum
+ * csum_ip4_header() - Calculate IPv4 header checksum
* @ip4h: IPv4 header
*/
-void csum_ip4_header(struct iphdr *ip4h)
+uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
+ uint32_t saddr, uint32_t daddr)
{
- ip4h->check = 0;
- ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
+ uint32_t sum = L2_BUF_IP4_PSUM(protocol);
+
+ sum += tot_len;
+ sum += (saddr >> 16) & 0xffff;
+ sum += saddr & 0xffff;
+ sum += (daddr >> 16) & 0xffff;
+ sum += daddr & 0xffff;
+
+ return ~csum_fold(sum);
}
/**
diff --git a/checksum.h b/checksum.h
index dfa705a04a24..92db73612b6e 100644
--- a/checksum.h
+++ b/checksum.h
@@ -13,7 +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_ip4_header(struct iphdr *ip4h);
+uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
+ uint32_t saddr, uint32_t daddr);
void csum_udp4(struct udphdr *udp4hr,
struct in_addr saddr, struct in_addr daddr,
const void *payload, size_t len);
diff --git a/tap.c b/tap.c
index d35d8944fc41..d4649f0167ab 100644
--- a/tap.c
+++ b/tap.c
@@ -161,7 +161,8 @@ static void *tap_push_ip4h(char *buf, struct in_addr src, struct in_addr dst,
ip4h->protocol = proto;
ip4h->saddr = src.s_addr;
ip4h->daddr = dst.s_addr;
- csum_ip4_header(ip4h);
+ ip4h->check = csum_ip4_header(ip4h->tot_len, proto,
+ src.s_addr, dst.s_addr);
return ip4h + 1;
}
diff --git a/tcp.c b/tcp.c
index e0588f92e65f..ea0802c6b102 100644
--- a/tcp.c
+++ b/tcp.c
@@ -935,23 +935,6 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
trace("TCP: failed to set SO_SNDBUF to %i", v);
}
-/**
- * tcp_update_check_ip4() - Update IPv4 with variable parts from stored one
- * @buf: L2 packet buffer with final IPv4 header
- */
-static void tcp_update_check_ip4(struct tcp4_l2_buf_t *buf)
-{
- uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_TCP);
-
- sum += buf->iph.tot_len;
- sum += (buf->iph.saddr >> 16) & 0xffff;
- sum += buf->iph.saddr & 0xffff;
- sum += (buf->iph.daddr >> 16) & 0xffff;
- sum += buf->iph.daddr & 0xffff;
-
- buf->iph.check = (uint16_t)~csum_fold(sum);
-}
-
/**
* tcp_update_check_tcp4() - Update TCP checksum from stored one
* @buf: L2 packet buffer with final IPv4 header
@@ -1394,10 +1377,9 @@ do { \
b->iph.saddr = a4->s_addr;
b->iph.daddr = c->ip4.addr_seen.s_addr;
- if (check)
- b->iph.check = *check;
- else
- tcp_update_check_ip4(b);
+ b->iph.check = check ? *check :
+ csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
+ b->iph.saddr, b->iph.daddr);
SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
diff --git a/udp.c b/udp.c
index 26774df7018c..d517c99dcc69 100644
--- a/udp.c
+++ b/udp.c
@@ -275,23 +275,6 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
}
}
-/**
- * udp_update_check4() - Update checksum with variable parts from stored one
- * @buf: L2 packet buffer with final IPv4 header
- */
-static void udp_update_check4(struct udp4_l2_buf_t *buf)
-{
- uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_UDP);
-
- sum += buf->iph.tot_len;
- sum += (buf->iph.saddr >> 16) & 0xffff;
- sum += buf->iph.saddr & 0xffff;
- sum += (buf->iph.daddr >> 16) & 0xffff;
- sum += buf->iph.daddr & 0xffff;
-
- buf->iph.check = (uint16_t)~csum_fold(sum);
-}
-
/**
* udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
* @eth_d: Ethernet destination address, NULL if unchanged
@@ -619,7 +602,8 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
b->iph.saddr = src->s_addr;
}
- udp_update_check4(b);
+ b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
+ b->iph.saddr, b->iph.daddr);
b->uh.source = b->s_in.sin_port;
b->uh.dest = htons(dstport);
b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
--
@@ -275,23 +275,6 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
}
}
-/**
- * udp_update_check4() - Update checksum with variable parts from stored one
- * @buf: L2 packet buffer with final IPv4 header
- */
-static void udp_update_check4(struct udp4_l2_buf_t *buf)
-{
- uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_UDP);
-
- sum += buf->iph.tot_len;
- sum += (buf->iph.saddr >> 16) & 0xffff;
- sum += buf->iph.saddr & 0xffff;
- sum += (buf->iph.daddr >> 16) & 0xffff;
- sum += buf->iph.daddr & 0xffff;
-
- buf->iph.check = (uint16_t)~csum_fold(sum);
-}
-
/**
* udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
* @eth_d: Ethernet destination address, NULL if unchanged
@@ -619,7 +602,8 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
b->iph.saddr = src->s_addr;
}
- udp_update_check4(b);
+ b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
+ b->iph.saddr, b->iph.daddr);
b->uh.source = b->s_in.sin_port;
b->uh.dest = htons(dstport);
b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
` (4 preceding siblings ...)
2024-02-29 16:59 ` [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 23:42 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 7/8] tap: make tap_update_mac() generic Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
7 siblings, 1 reply; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier
The TCP and UDP checksums are computed using the data in the TCP/UDP
payload but also some informations in the IP header (protocol,
length, source and destination addresses).
We add two functions, proto_ipv4_header_psum() and
proto_ipv6_header_psum(), to compute the checksum of the IP
header part.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v4:
- fix payload length endianness
v3:
- function parameters provide tot_len, saddr, daddr and protocol
rather than an iphdr/ipv6hdr
v2:
- move new function to checksum.c
- use _psum rather than _checksum in the name
- replace csum_udp4() and csum_udp6() by the new function
checksum.c | 69 ++++++++++++++++++++++++++++++++++++++++++------------
checksum.h | 4 ++++
tcp.c | 45 ++++++++++++++++-------------------
udp.c | 13 ++++++----
4 files changed, 86 insertions(+), 45 deletions(-)
diff --git a/checksum.c b/checksum.c
index 511b296a9a80..93c8d5205c2b 100644
--- a/checksum.c
+++ b/checksum.c
@@ -134,6 +134,30 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
return ~csum_fold(sum);
}
+/**
+ * proto_ipv4_header_psum() - Calculates the partial checksum of an
+ * IPv4 header for UDP or TCP
+ * @tot_len: IPv4 Payload length
+ * @proto: Protocol number
+ * @saddr: Source address
+ * @daddr: Destination address
+ * @proto: proto Protocol number
+ * Returns: Partial checksum of the IPv4 header
+ */
+uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol,
+ uint32_t saddr, uint32_t daddr)
+{
+ uint32_t psum = htons(protocol);
+
+ psum += (saddr >> 16) & 0xffff;
+ psum += saddr & 0xffff;
+ psum += (daddr >> 16) & 0xffff;
+ psum += daddr & 0xffff;
+ psum += htons(tot_len);
+
+ return psum;
+}
+
/**
* csum_udp4() - Calculate and set checksum for a UDP over IPv4 packet
* @udp4hr: UDP header, initialised apart from checksum
@@ -150,14 +174,12 @@ void csum_udp4(struct udphdr *udp4hr,
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(saddr.s_addr)
- + csum_fold(daddr.s_addr)
- + htons(len + sizeof(*udp4hr))
- + htons(IPPROTO_UDP);
- /* Add in partial checksum for the UDP header alone */
- psum += sum_16b(udp4hr, sizeof(*udp4hr));
+ uint16_t tot_len = len + sizeof(struct udphdr);
+ uint32_t psum = proto_ipv4_header_psum(tot_len,
+ IPPROTO_UDP,
+ saddr.s_addr,
+ daddr.s_addr);
+ psum = csum_unfolded(udp4hr, sizeof(struct udphdr), psum);
udp4hr->check = csum(payload, len, psum);
}
}
@@ -180,6 +202,26 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)
icmp4hr->checksum = csum(payload, len, psum);
}
+/**
+ * proto_ipv6_header_psum() - Calculates the partial checksum of an
+ * IPv6 header for UDP or TCP
+ * @payload_len: IPv6 payload length
+ * @proto: Protocol number
+ * @saddr: Source address
+ * @daddr: Destination address
+ * Returns: Partial checksum of the IPv6 header
+ */
+uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
+ struct in6_addr saddr, struct in6_addr daddr)
+{
+ uint32_t sum = htons(protocol) + htons(payload_len);
+
+ sum += sum_16b(&saddr, sizeof(saddr));
+ sum += sum_16b(&daddr, sizeof(daddr));
+
+ return sum;
+}
+
/**
* csum_udp6() - Calculate and set checksum for a UDP over IPv6 packet
* @udp6hr: UDP header, initialised apart from checksum
@@ -190,14 +232,11 @@ void csum_udp6(struct udphdr *udp6hr,
const struct in6_addr *saddr, const struct in6_addr *daddr,
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);
-
+ uint32_t psum = proto_ipv6_header_psum(len + sizeof(struct udphdr),
+ IPPROTO_UDP, *saddr, *daddr);
udp6hr->check = 0;
- /* Add in partial checksum for the UDP header alone */
- psum += sum_16b(udp6hr, sizeof(*udp6hr));
+
+ psum = csum_unfolded(udp6hr, sizeof(struct udphdr), psum);
udp6hr->check = csum(payload, len, psum);
}
diff --git a/checksum.h b/checksum.h
index 92db73612b6e..b2b5b8e8b77e 100644
--- a/checksum.h
+++ b/checksum.h
@@ -15,10 +15,14 @@ uint16_t csum_fold(uint32_t sum);
uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init);
uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
uint32_t saddr, uint32_t daddr);
+uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol,
+ uint32_t saddr, uint32_t daddr);
void csum_udp4(struct udphdr *udp4hr,
struct in_addr saddr, struct in_addr daddr,
const void *payload, size_t len);
void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len);
+uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
+ struct in6_addr saddr, struct in6_addr daddr);
void csum_udp6(struct udphdr *udp6hr,
const struct in6_addr *saddr, const struct in6_addr *daddr,
const void *payload, size_t len);
diff --git a/tcp.c b/tcp.c
index ea0802c6b102..d78efa5401bb 100644
--- a/tcp.c
+++ b/tcp.c
@@ -939,39 +939,30 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
* tcp_update_check_tcp4() - Update TCP checksum from stored one
* @buf: L2 packet buffer with final IPv4 header
*/
-static void tcp_update_check_tcp4(struct tcp4_l2_buf_t *buf)
+static void tcp_update_check_tcp4(struct iphdr *iph)
{
- uint16_t tlen = ntohs(buf->iph.tot_len) - 20;
- uint32_t sum = htons(IPPROTO_TCP);
+ uint16_t tlen = ntohs(iph->tot_len) - sizeof(struct iphdr);
+ uint32_t sum = proto_ipv4_header_psum(tlen, IPPROTO_TCP,
+ iph->saddr, iph->daddr);
+ struct tcphdr *th = (struct tcphdr *)(iph + 1);
- sum += (buf->iph.saddr >> 16) & 0xffff;
- sum += buf->iph.saddr & 0xffff;
- sum += (buf->iph.daddr >> 16) & 0xffff;
- sum += buf->iph.daddr & 0xffff;
- sum += htons(ntohs(buf->iph.tot_len) - 20);
-
- buf->th.check = 0;
- buf->th.check = csum(&buf->th, tlen, sum);
+ th->check = 0;
+ th->check = csum(th, tlen, sum);
}
/**
* tcp_update_check_tcp6() - Calculate TCP checksum for IPv6
* @buf: L2 packet buffer with final IPv6 header
*/
-static void tcp_update_check_tcp6(struct tcp6_l2_buf_t *buf)
+static void tcp_update_check_tcp6(struct ipv6hdr *ip6h)
{
- int len = ntohs(buf->ip6h.payload_len) + sizeof(struct ipv6hdr);
-
- buf->ip6h.hop_limit = IPPROTO_TCP;
- buf->ip6h.version = 0;
- buf->ip6h.nexthdr = 0;
+ struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
+ uint16_t payload_len = ntohs(ip6h->payload_len);
+ uint32_t sum = proto_ipv6_header_psum(payload_len, IPPROTO_TCP,
+ ip6h->saddr, ip6h->daddr);
- buf->th.check = 0;
- buf->th.check = csum(&buf->ip6h, len, 0);
-
- buf->ip6h.hop_limit = 255;
- buf->ip6h.version = 6;
- buf->ip6h.nexthdr = IPPROTO_TCP;
+ th->check = 0;
+ th->check = csum(th, payload_len, sum);
}
/**
@@ -1383,7 +1374,7 @@ do { \
SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
- tcp_update_check_tcp4(b);
+ tcp_update_check_tcp4(&b->iph);
tlen = tap_iov_len(c, &b->taph, ip_len);
} else {
@@ -1402,7 +1393,11 @@ do { \
SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
- tcp_update_check_tcp6(b);
+ tcp_update_check_tcp6(&b->ip6h);
+
+ b->ip6h.hop_limit = 255;
+ b->ip6h.version = 6;
+ b->ip6h.nexthdr = IPPROTO_TCP;
b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
diff --git a/udp.c b/udp.c
index d517c99dcc69..410ace16a6a2 100644
--- a/udp.c
+++ b/udp.c
@@ -625,6 +625,7 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
{
struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
struct in6_addr *src;
+ uint16_t payload_len;
in_port_t src_port;
size_t ip_len;
@@ -633,7 +634,8 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
- b->ip6h.payload_len = htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh));
+ payload_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->uh);
+ b->ip6h.payload_len = htons(payload_len);
if (IN6_IS_ADDR_LINKLOCAL(src)) {
b->ip6h.daddr = c->ip6.addr_ll_seen;
@@ -675,10 +677,11 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
b->uh.source = b->s_in6.sin6_port;
b->uh.dest = htons(dstport);
b->uh.len = b->ip6h.payload_len;
-
- b->ip6h.hop_limit = IPPROTO_UDP;
- b->ip6h.version = b->ip6h.nexthdr = b->uh.check = 0;
- b->uh.check = csum(&b->ip6h, ip_len, 0);
+ b->uh.check = 0;
+ b->uh.check = csum(&b->uh, payload_len,
+ proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
+ b->ip6h.saddr,
+ b->ip6h.daddr));
b->ip6h.version = 6;
b->ip6h.nexthdr = IPPROTO_UDP;
b->ip6h.hop_limit = 255;
--
@@ -625,6 +625,7 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
{
struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
struct in6_addr *src;
+ uint16_t payload_len;
in_port_t src_port;
size_t ip_len;
@@ -633,7 +634,8 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
- b->ip6h.payload_len = htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh));
+ payload_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->uh);
+ b->ip6h.payload_len = htons(payload_len);
if (IN6_IS_ADDR_LINKLOCAL(src)) {
b->ip6h.daddr = c->ip6.addr_ll_seen;
@@ -675,10 +677,11 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
b->uh.source = b->s_in6.sin6_port;
b->uh.dest = htons(dstport);
b->uh.len = b->ip6h.payload_len;
-
- b->ip6h.hop_limit = IPPROTO_UDP;
- b->ip6h.version = b->ip6h.nexthdr = b->uh.check = 0;
- b->uh.check = csum(&b->ip6h, ip_len, 0);
+ b->uh.check = 0;
+ b->uh.check = csum(&b->uh, payload_len,
+ proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
+ b->ip6h.saddr,
+ b->ip6h.daddr));
b->ip6h.version = 6;
b->ip6h.nexthdr = IPPROTO_UDP;
b->ip6h.hop_limit = 255;
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 7/8] tap: make tap_update_mac() generic
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
` (5 preceding siblings ...)
2024-02-29 16:59 ` [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
7 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier, David Gibson
Use ethhdr rather than tap_hdr.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
Notes:
v4:
- rebase
v3:
- add David's R-b
v2:
- update function comment
- move the patch earlier in the series
tap.c | 10 +++++-----
tap.h | 2 +-
tcp.c | 8 ++++----
udp.c | 4 ++--
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/tap.c b/tap.c
index d4649f0167ab..48574f941958 100644
--- a/tap.c
+++ b/tap.c
@@ -420,18 +420,18 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, size_t n)
}
/**
- * tap_update_mac() - Update tap L2 header with new Ethernet addresses
- * @taph: Tap headers to update
+ * eth_update_mac() - Update tap L2 header with new Ethernet addresses
+ * @eh: Ethernet headers to update
* @eth_d: Ethernet destination address, NULL if unchanged
* @eth_s: Ethernet source address, NULL if unchanged
*/
-void tap_update_mac(struct tap_hdr *taph,
+void eth_update_mac(struct ethhdr *eh,
const unsigned char *eth_d, const unsigned char *eth_s)
{
if (eth_d)
- memcpy(taph->eh.h_dest, eth_d, sizeof(taph->eh.h_dest));
+ memcpy(eh->h_dest, eth_d, sizeof(eh->h_dest));
if (eth_s)
- memcpy(taph->eh.h_source, eth_s, sizeof(taph->eh.h_source));
+ memcpy(eh->h_source, eth_s, sizeof(eh->h_source));
}
PACKET_POOL_DECL(pool_l4, UIO_MAXIOV, pkt_buf);
diff --git a/tap.h b/tap.h
index 466d91466c3d..437b9aa2b43f 100644
--- a/tap.h
+++ b/tap.h
@@ -74,7 +74,7 @@ void tap_icmp6_send(const struct ctx *c,
const void *in, size_t len);
int tap_send(const struct ctx *c, const void *data, size_t len);
size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, size_t n);
-void tap_update_mac(struct tap_hdr *taph,
+void eth_update_mac(struct ethhdr *eh,
const unsigned char *eth_d, const unsigned char *eth_s);
void tap_listen_handler(struct ctx *c, uint32_t events);
void tap_handler_pasta(struct ctx *c, uint32_t events,
diff --git a/tcp.c b/tcp.c
index d78efa5401bb..5b2fdf662a6c 100644
--- a/tcp.c
+++ b/tcp.c
@@ -980,10 +980,10 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
struct tcp4_l2_buf_t *b4 = &tcp4_l2_buf[i];
struct tcp6_l2_buf_t *b6 = &tcp6_l2_buf[i];
- tap_update_mac(&b4->taph, eth_d, eth_s);
- tap_update_mac(&b6->taph, eth_d, eth_s);
- tap_update_mac(&b4f->taph, eth_d, eth_s);
- tap_update_mac(&b6f->taph, eth_d, eth_s);
+ eth_update_mac(&b4->taph.eh, eth_d, eth_s);
+ eth_update_mac(&b6->taph.eh, eth_d, eth_s);
+ eth_update_mac(&b4f->taph.eh, eth_d, eth_s);
+ eth_update_mac(&b6f->taph.eh, eth_d, eth_s);
}
}
diff --git a/udp.c b/udp.c
index 410ace16a6a2..2aff428a009a 100644
--- a/udp.c
+++ b/udp.c
@@ -288,8 +288,8 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
struct udp4_l2_buf_t *b4 = &udp4_l2_buf[i];
struct udp6_l2_buf_t *b6 = &udp6_l2_buf[i];
- tap_update_mac(&b4->taph, eth_d, eth_s);
- tap_update_mac(&b6->taph, eth_d, eth_s);
+ eth_update_mac(&b4->taph.eh, eth_d, eth_s);
+ eth_update_mac(&b6->taph.eh, eth_d, eth_s);
}
}
--
@@ -288,8 +288,8 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
struct udp4_l2_buf_t *b4 = &udp4_l2_buf[i];
struct udp6_l2_buf_t *b6 = &udp6_l2_buf[i];
- tap_update_mac(&b4->taph, eth_d, eth_s);
- tap_update_mac(&b6->taph, eth_d, eth_s);
+ eth_update_mac(&b4->taph.eh, eth_d, eth_s);
+ eth_update_mac(&b6->taph.eh, eth_d, eth_s);
}
}
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
` (6 preceding siblings ...)
2024-02-29 16:59 ` [PATCH v4 7/8] tap: make tap_update_mac() generic Laurent Vivier
@ 2024-02-29 16:59 ` Laurent Vivier
2024-02-29 23:54 ` David Gibson
7 siblings, 1 reply; 17+ messages in thread
From: Laurent Vivier @ 2024-02-29 16:59 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier
Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function
tcp_fill_header().
Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to
tcp_fill_ipv4_header() and tcp_fill_ipv6_header()
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v4:
- group all the ip6g initialisations together and
remove flow_lbl preset to 0
- add ASSERT(a4)
v3:
- add to sub-series part 1
v2:
- extract header filling functions from
"tcp: extract buffer management from tcp_send_flag()"
- rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(),
tcp_fill_ipv6_header()
- use upside-down Christmas tree arguments order
- replace (void *) by (struct tcphdr *)
tcp.c | 154 +++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 104 insertions(+), 50 deletions(-)
diff --git a/tcp.c b/tcp.c
index 5b2fdf662a6c..ced22534a103 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c)
tcp_l2_data_buf_flush(c);
}
+/**
+ * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
+ *
+ * @th: Pointer to the TCP header structure
+ * @conn: Pointer to the TCP connection structure
+ * @seq: Sequence number
+ */
+static void tcp_fill_header(struct tcphdr *th,
+ const struct tcp_tap_conn *conn, uint32_t seq)
+{
+ th->source = htons(conn->fport);
+ th->dest = htons(conn->eport);
+ th->seq = htonl(seq);
+ th->ack_seq = htonl(conn->seq_ack_to_tap);
+ if (conn->events & ESTABLISHED) {
+ th->window = htons(conn->wnd_to_tap);
+ } else {
+ unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
+
+ th->window = htons(MIN(wnd, USHRT_MAX));
+ }
+}
+
+/**
+ * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
+ * @c: Execution context
+ * @conn: Connection pointer
+ * @iph: Pointer to IPv4 header, immediately followed by a TCP header
+ * @plen: Payload length (including TCP header options)
+ * @check: Checksum, if already known
+ * @seq: Sequence number for this segment
+ *
+ * Return: IP frame length including L2 headers, host order
+ */
+static size_t tcp_fill_ipv4_header(const struct ctx *c,
+ const struct tcp_tap_conn *conn,
+ struct iphdr *iph, size_t plen,
+ const uint16_t *check, uint32_t seq)
+{
+ size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
+ const struct in_addr *a4 = inany_v4(&conn->faddr);
+ struct tcphdr *th = (struct tcphdr *)(iph + 1);
+
+ ASSERT(a4);
+
+ iph->tot_len = htons(ip_len);
+ iph->saddr = a4->s_addr;
+ iph->daddr = c->ip4.addr_seen.s_addr;
+
+ iph->check = check ? *check :
+ csum_ip4_header(iph->tot_len, IPPROTO_TCP,
+ iph->saddr, iph->daddr);
+
+
+ tcp_fill_header(th, conn, seq);
+
+ tcp_update_check_tcp4(iph);
+
+ return ip_len;
+}
+
+/**
+ * tcp_fill_ipv6_header() - Fill 802.3, IPv6, TCP headers in pre-cooked buffers
+ * @c: Execution context
+ * @conn: Connection pointer
+ * @ip6h: Pointer to IPv6 header, immediately followed by a TCP header
+ * @plen: Payload length (including TCP header options)
+ * @check: Checksum, if already known
+ * @seq: Sequence number for this segment
+ *
+ * Return: The total length of the IPv6 packet, host order
+ */
+static size_t tcp_fill_ipv6_header(const struct ctx *c,
+ const struct tcp_tap_conn *conn,
+ struct ipv6hdr *ip6h, size_t plen,
+ uint32_t seq)
+{
+ size_t ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
+ struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
+
+ ip6h->payload_len = htons(plen + sizeof(struct tcphdr));
+ ip6h->saddr = conn->faddr.a6;
+ if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
+ ip6h->daddr = c->ip6.addr_ll_seen;
+ else
+ ip6h->daddr = c->ip6.addr_seen;
+
+ ip6h->hop_limit = 255;
+ ip6h->version = 6;
+ ip6h->nexthdr = IPPROTO_TCP;
+
+ ip6h->flow_lbl[0] = (conn->sock >> 16) & 0xf;
+ ip6h->flow_lbl[1] = (conn->sock >> 8) & 0xff;
+ ip6h->flow_lbl[2] = (conn->sock >> 0) & 0xff;
+
+ tcp_fill_header(th, conn, seq);
+
+ tcp_update_check_tcp6(ip6h);
+
+ return ip_len;
+}
+
/**
* tcp_l2_buf_fill_headers() - Fill 802.3, IP, TCP headers in pre-cooked buffers
* @c: Execution context
@@ -1345,67 +1447,19 @@ static size_t tcp_l2_buf_fill_headers(const struct ctx *c,
const struct in_addr *a4 = inany_v4(&conn->faddr);
size_t ip_len, tlen;
-#define SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq) \
-do { \
- b->th.source = htons(conn->fport); \
- b->th.dest = htons(conn->eport); \
- b->th.seq = htonl(seq); \
- b->th.ack_seq = htonl(conn->seq_ack_to_tap); \
- if (conn->events & ESTABLISHED) { \
- b->th.window = htons(conn->wnd_to_tap); \
- } else { \
- unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap; \
- \
- b->th.window = htons(MIN(wnd, USHRT_MAX)); \
- } \
-} while (0)
-
if (a4) {
struct tcp4_l2_buf_t *b = (struct tcp4_l2_buf_t *)p;
- ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
- b->iph.tot_len = htons(ip_len);
- b->iph.saddr = a4->s_addr;
- b->iph.daddr = c->ip4.addr_seen.s_addr;
-
- b->iph.check = check ? *check :
- csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
- b->iph.saddr, b->iph.daddr);
-
- SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
-
- tcp_update_check_tcp4(&b->iph);
+ ip_len = tcp_fill_ipv4_header(c, conn, &b->iph, plen, check, seq);
tlen = tap_iov_len(c, &b->taph, ip_len);
} else {
struct tcp6_l2_buf_t *b = (struct tcp6_l2_buf_t *)p;
- ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
-
- b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
- b->ip6h.saddr = conn->faddr.a6;
- if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
- b->ip6h.daddr = c->ip6.addr_ll_seen;
- else
- b->ip6h.daddr = c->ip6.addr_seen;
-
- memset(b->ip6h.flow_lbl, 0, 3);
-
- SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
-
- tcp_update_check_tcp6(&b->ip6h);
-
- b->ip6h.hop_limit = 255;
- b->ip6h.version = 6;
- b->ip6h.nexthdr = IPPROTO_TCP;
-
- b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
- b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
- b->ip6h.flow_lbl[2] = (conn->sock >> 0) & 0xff;
+ ip_len = tcp_fill_ipv6_header(c, conn, &b->ip6h, plen, seq);
tlen = tap_iov_len(c, &b->taph, ip_len);
}
-#undef SET_TCP_HEADER_COMMON_V4_V6
return tlen;
}
--
@@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c)
tcp_l2_data_buf_flush(c);
}
+/**
+ * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
+ *
+ * @th: Pointer to the TCP header structure
+ * @conn: Pointer to the TCP connection structure
+ * @seq: Sequence number
+ */
+static void tcp_fill_header(struct tcphdr *th,
+ const struct tcp_tap_conn *conn, uint32_t seq)
+{
+ th->source = htons(conn->fport);
+ th->dest = htons(conn->eport);
+ th->seq = htonl(seq);
+ th->ack_seq = htonl(conn->seq_ack_to_tap);
+ if (conn->events & ESTABLISHED) {
+ th->window = htons(conn->wnd_to_tap);
+ } else {
+ unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
+
+ th->window = htons(MIN(wnd, USHRT_MAX));
+ }
+}
+
+/**
+ * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
+ * @c: Execution context
+ * @conn: Connection pointer
+ * @iph: Pointer to IPv4 header, immediately followed by a TCP header
+ * @plen: Payload length (including TCP header options)
+ * @check: Checksum, if already known
+ * @seq: Sequence number for this segment
+ *
+ * Return: IP frame length including L2 headers, host order
+ */
+static size_t tcp_fill_ipv4_header(const struct ctx *c,
+ const struct tcp_tap_conn *conn,
+ struct iphdr *iph, size_t plen,
+ const uint16_t *check, uint32_t seq)
+{
+ size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
+ const struct in_addr *a4 = inany_v4(&conn->faddr);
+ struct tcphdr *th = (struct tcphdr *)(iph + 1);
+
+ ASSERT(a4);
+
+ iph->tot_len = htons(ip_len);
+ iph->saddr = a4->s_addr;
+ iph->daddr = c->ip4.addr_seen.s_addr;
+
+ iph->check = check ? *check :
+ csum_ip4_header(iph->tot_len, IPPROTO_TCP,
+ iph->saddr, iph->daddr);
+
+
+ tcp_fill_header(th, conn, seq);
+
+ tcp_update_check_tcp4(iph);
+
+ return ip_len;
+}
+
+/**
+ * tcp_fill_ipv6_header() - Fill 802.3, IPv6, TCP headers in pre-cooked buffers
+ * @c: Execution context
+ * @conn: Connection pointer
+ * @ip6h: Pointer to IPv6 header, immediately followed by a TCP header
+ * @plen: Payload length (including TCP header options)
+ * @check: Checksum, if already known
+ * @seq: Sequence number for this segment
+ *
+ * Return: The total length of the IPv6 packet, host order
+ */
+static size_t tcp_fill_ipv6_header(const struct ctx *c,
+ const struct tcp_tap_conn *conn,
+ struct ipv6hdr *ip6h, size_t plen,
+ uint32_t seq)
+{
+ size_t ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
+ struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
+
+ ip6h->payload_len = htons(plen + sizeof(struct tcphdr));
+ ip6h->saddr = conn->faddr.a6;
+ if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
+ ip6h->daddr = c->ip6.addr_ll_seen;
+ else
+ ip6h->daddr = c->ip6.addr_seen;
+
+ ip6h->hop_limit = 255;
+ ip6h->version = 6;
+ ip6h->nexthdr = IPPROTO_TCP;
+
+ ip6h->flow_lbl[0] = (conn->sock >> 16) & 0xf;
+ ip6h->flow_lbl[1] = (conn->sock >> 8) & 0xff;
+ ip6h->flow_lbl[2] = (conn->sock >> 0) & 0xff;
+
+ tcp_fill_header(th, conn, seq);
+
+ tcp_update_check_tcp6(ip6h);
+
+ return ip_len;
+}
+
/**
* tcp_l2_buf_fill_headers() - Fill 802.3, IP, TCP headers in pre-cooked buffers
* @c: Execution context
@@ -1345,67 +1447,19 @@ static size_t tcp_l2_buf_fill_headers(const struct ctx *c,
const struct in_addr *a4 = inany_v4(&conn->faddr);
size_t ip_len, tlen;
-#define SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq) \
-do { \
- b->th.source = htons(conn->fport); \
- b->th.dest = htons(conn->eport); \
- b->th.seq = htonl(seq); \
- b->th.ack_seq = htonl(conn->seq_ack_to_tap); \
- if (conn->events & ESTABLISHED) { \
- b->th.window = htons(conn->wnd_to_tap); \
- } else { \
- unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap; \
- \
- b->th.window = htons(MIN(wnd, USHRT_MAX)); \
- } \
-} while (0)
-
if (a4) {
struct tcp4_l2_buf_t *b = (struct tcp4_l2_buf_t *)p;
- ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
- b->iph.tot_len = htons(ip_len);
- b->iph.saddr = a4->s_addr;
- b->iph.daddr = c->ip4.addr_seen.s_addr;
-
- b->iph.check = check ? *check :
- csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
- b->iph.saddr, b->iph.daddr);
-
- SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
-
- tcp_update_check_tcp4(&b->iph);
+ ip_len = tcp_fill_ipv4_header(c, conn, &b->iph, plen, check, seq);
tlen = tap_iov_len(c, &b->taph, ip_len);
} else {
struct tcp6_l2_buf_t *b = (struct tcp6_l2_buf_t *)p;
- ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
-
- b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
- b->ip6h.saddr = conn->faddr.a6;
- if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
- b->ip6h.daddr = c->ip6.addr_ll_seen;
- else
- b->ip6h.daddr = c->ip6.addr_seen;
-
- memset(b->ip6h.flow_lbl, 0, 3);
-
- SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
-
- tcp_update_check_tcp6(&b->ip6h);
-
- b->ip6h.hop_limit = 255;
- b->ip6h.version = 6;
- b->ip6h.nexthdr = IPPROTO_TCP;
-
- b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
- b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
- b->ip6h.flow_lbl[2] = (conn->sock >> 0) & 0xff;
+ ip_len = tcp_fill_ipv6_header(c, conn, &b->ip6h, plen, seq);
tlen = tap_iov_len(c, &b->taph, ip_len);
}
-#undef SET_TCP_HEADER_COMMON_V4_V6
return tlen;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/8] pcap: add pcap_iov()
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
@ 2024-02-29 23:16 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-02-29 23:16 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]
On Thu, Feb 29, 2024 at 05:59:48PM +0100, Laurent Vivier wrote:
> Introduce a new function pcap_iov() to capture packet desribed by an IO
> vector.
>
> Update pcap_frame() to manage iovcnt > 1.
Yikes. I hadn't actually realised my version only worked for iovcnt
== 1.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>
> Notes:
> v4:
> - use pcap_frame()
>
> v3:
> - update rationale
> - update comment
> - use strerror(errno)
> - use size_t for io vector length
>
> v2:
> - introduce pcap_header(), a common helper to write
> packet header
> - use writev() rather than write() in a loop
> - add functions comment
>
> pcap.c | 26 ++++++++++++++++++++++----
> pcap.h | 1 +
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/pcap.c b/pcap.c
> index a4057b5f9c6a..372f02045262 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -32,6 +32,7 @@
> #include "passt.h"
> #include "log.h"
> #include "pcap.h"
> +#include "iov.h"
>
> #define PCAP_VERSION_MINOR 4
>
> @@ -78,7 +79,7 @@ struct pcap_pkthdr {
> static void pcap_frame(const struct iovec *iov, size_t iovcnt,
> size_t offset, const struct timeval *tv)
> {
> - size_t len = iov->iov_len - offset;
> + size_t len = iov_size(iov, iovcnt) - offset;
> struct pcap_pkthdr h = {
> .tv_sec = tv->tv_sec,
> .tv_usec = tv->tv_usec,
> @@ -87,10 +88,8 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
> };
> struct iovec hiov = { &h, sizeof(h) };
>
> - (void)iovcnt;
> -
> if (write_remainder(pcap_fd, &hiov, 1, 0) < 0 ||
> - write_remainder(pcap_fd, iov, 1, offset) < 0) {
> + write_remainder(pcap_fd, iov, iovcnt, offset) < 0) {
> debug("Cannot log packet, length %zu: %s",
> len, strerror(errno));
> }
> @@ -135,6 +134,25 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
> pcap_frame(iov + i * frame_parts, frame_parts, offset, &tv);
> }
>
> +/*
> + * pcap_iov - Write packet data described by an I/O vector
> + * to a pcap file descriptor.
> + *
> + * @iov: Pointer to the array of struct iovec describing the I/O vector
> + * containing packet data to write, including L2 header
> + * @iovcnt: Number of buffers (@iov entries)
> + */
> +void pcap_iov(const struct iovec *iov, size_t iovcnt)
> +{
> + struct timeval tv;
> +
> + if (pcap_fd == -1)
> + return;
> +
> + gettimeofday(&tv, NULL);
> + pcap_frame(iov, iovcnt, 0, &tv);
> +}
> +
> /**
> * pcap_init() - Initialise pcap file
> * @c: Execution context
> diff --git a/pcap.h b/pcap.h
> index 85fc58e57572..b1c4c909c109 100644
> --- a/pcap.h
> +++ b/pcap.h
> @@ -9,6 +9,7 @@
> void pcap(const char *pkt, size_t len);
> void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
> size_t offset);
> +void pcap_iov(const struct iovec *iov, size_t n);
> void pcap_init(struct ctx *c);
>
> #endif /* PCAP_H */
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch]
2024-02-29 16:59 ` [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
@ 2024-02-29 23:19 ` David Gibson
2024-03-01 12:06 ` Laurent Vivier
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2024-02-29 23:19 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On Thu, Feb 29, 2024 at 05:59:51PM +0100, Laurent Vivier wrote:
> Introduce ip.[ch] file to encapsulate IP protocol handling functions and
> structures. Modify various files to include the new header ip.h when
> it's needed.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>
> Notes:
> v4:
> - rebase
>
> v3:
> - rewrap rationale
> - add David's R-b
>
> v2:
> - update rationale and comments
>
> Makefile | 10 +++---
> conf.c | 1 +
> dhcp.c | 1 +
> flow.c | 1 +
> fwd.c | 1 +
> icmp.c | 1 +
> inany.c | 1 +
> ip.c | 72 +++++++++++++++++++++++++++++++++++++++++++
> ip.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
Noe that ip.h exists, in4addr_loopback and in4addr_any should probably
go in there rather than inany.h. That can be a followup change
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c
2024-02-29 16:59 ` [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
@ 2024-02-29 23:25 ` David Gibson
2024-03-01 12:31 ` Laurent Vivier
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2024-02-29 23:25 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 6460 bytes --]
On Thu, Feb 29, 2024 at 05:59:52PM +0100, Laurent Vivier wrote:
> We can find the same function to compute the IPv4 header
> checksum in tcp.c, udp.c and tap.c
>
> Use the function defined for tap.c, csum_ip4_header(), but
> with the code used in tcp.c and udp.c as it doesn't need a fully
> initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>
> Notes:
> v4:
> - rebase
>
> v3:
> - function parameters provide tot_len, saddr, daddr and protocol
> rather than an iphdr
>
> v2:
> - use csum_ip4_header() from checksum.c
> - use code from tcp.c and udp.c in csum_ip4_header()
> - use "const struct iphfr *", check is not updated by the
> function but by the caller.
>
> checksum.c | 17 +++++++++++++----
> checksum.h | 3 ++-
> tap.c | 3 ++-
> tcp.c | 24 +++---------------------
> udp.c | 20 ++------------------
> 5 files changed, 22 insertions(+), 45 deletions(-)
>
> diff --git a/checksum.c b/checksum.c
> index 74e3742bc6f6..511b296a9a80 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -57,6 +57,7 @@
> #include <linux/icmpv6.h>
>
> #include "util.h"
> +#include "ip.h"
> #include "checksum.h"
>
> /* Checksums are optional for UDP over IPv4, so we usually just set
> @@ -116,13 +117,21 @@ uint16_t csum_fold(uint32_t sum)
> uint16_t csum(const void *buf, size_t len, uint32_t init);
>
> /**
> - * csum_ip4_header() - Calculate and set IPv4 header checksum
> + * csum_ip4_header() - Calculate IPv4 header checksum
> * @ip4h: IPv4 header
Function comment needs to be updated for the new parameters. In
particular it needs to note that tot_len, saddr and daddr are all
passed in network order.
As noted elsewhere, I kind of hate passing non-host-endian values in
plain integer types, but I can see why doing otherwise here would be
very awkward.
> */
> -void csum_ip4_header(struct iphdr *ip4h)
> +uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
> + uint32_t saddr, uint32_t daddr)
> {
> - ip4h->check = 0;
> - ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0);
> + uint32_t sum = L2_BUF_IP4_PSUM(protocol);
> +
> + sum += tot_len;
> + sum += (saddr >> 16) & 0xffff;
> + sum += saddr & 0xffff;
> + sum += (daddr >> 16) & 0xffff;
> + sum += daddr & 0xffff;
> +
> + return ~csum_fold(sum);
> }
>
> /**
> diff --git a/checksum.h b/checksum.h
> index dfa705a04a24..92db73612b6e 100644
> --- a/checksum.h
> +++ b/checksum.h
> @@ -13,7 +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_ip4_header(struct iphdr *ip4h);
> +uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
> + uint32_t saddr, uint32_t daddr);
> void csum_udp4(struct udphdr *udp4hr,
> struct in_addr saddr, struct in_addr daddr,
> const void *payload, size_t len);
> diff --git a/tap.c b/tap.c
> index d35d8944fc41..d4649f0167ab 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -161,7 +161,8 @@ static void *tap_push_ip4h(char *buf, struct in_addr src, struct in_addr dst,
> ip4h->protocol = proto;
> ip4h->saddr = src.s_addr;
> ip4h->daddr = dst.s_addr;
> - csum_ip4_header(ip4h);
> + ip4h->check = csum_ip4_header(ip4h->tot_len, proto,
> + src.s_addr, dst.s_addr);
> return ip4h + 1;
> }
>
> diff --git a/tcp.c b/tcp.c
> index e0588f92e65f..ea0802c6b102 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -935,23 +935,6 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
> trace("TCP: failed to set SO_SNDBUF to %i", v);
> }
>
> -/**
> - * tcp_update_check_ip4() - Update IPv4 with variable parts from stored one
> - * @buf: L2 packet buffer with final IPv4 header
> - */
> -static void tcp_update_check_ip4(struct tcp4_l2_buf_t *buf)
> -{
> - uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_TCP);
> -
> - sum += buf->iph.tot_len;
> - sum += (buf->iph.saddr >> 16) & 0xffff;
> - sum += buf->iph.saddr & 0xffff;
> - sum += (buf->iph.daddr >> 16) & 0xffff;
> - sum += buf->iph.daddr & 0xffff;
> -
> - buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
> /**
> * tcp_update_check_tcp4() - Update TCP checksum from stored one
> * @buf: L2 packet buffer with final IPv4 header
> @@ -1394,10 +1377,9 @@ do { \
> b->iph.saddr = a4->s_addr;
> b->iph.daddr = c->ip4.addr_seen.s_addr;
>
> - if (check)
> - b->iph.check = *check;
> - else
> - tcp_update_check_ip4(b);
> + b->iph.check = check ? *check :
> + csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
> + b->iph.saddr, b->iph.daddr);
>
> SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>
> diff --git a/udp.c b/udp.c
> index 26774df7018c..d517c99dcc69 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -275,23 +275,6 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
> }
> }
>
> -/**
> - * udp_update_check4() - Update checksum with variable parts from stored one
> - * @buf: L2 packet buffer with final IPv4 header
> - */
> -static void udp_update_check4(struct udp4_l2_buf_t *buf)
> -{
> - uint32_t sum = L2_BUF_IP4_PSUM(IPPROTO_UDP);
> -
> - sum += buf->iph.tot_len;
> - sum += (buf->iph.saddr >> 16) & 0xffff;
> - sum += buf->iph.saddr & 0xffff;
> - sum += (buf->iph.daddr >> 16) & 0xffff;
> - sum += buf->iph.daddr & 0xffff;
> -
> - buf->iph.check = (uint16_t)~csum_fold(sum);
> -}
> -
> /**
> * udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
> * @eth_d: Ethernet destination address, NULL if unchanged
> @@ -619,7 +602,8 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
> b->iph.saddr = src->s_addr;
> }
>
> - udp_update_check4(b);
> + b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
> + b->iph.saddr, b->iph.daddr);
> b->uh.source = b->s_in.sin_port;
> b->uh.dest = htons(dstport);
> b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh));
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP
2024-02-29 16:59 ` [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
@ 2024-02-29 23:42 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-02-29 23:42 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 11120 bytes --]
On Thu, Feb 29, 2024 at 05:59:53PM +0100, Laurent Vivier wrote:
> The TCP and UDP checksums are computed using the data in the TCP/UDP
> payload but also some informations in the IP header (protocol,
> length, source and destination addresses).
>
> We add two functions, proto_ipv4_header_psum() and
> proto_ipv6_header_psum(), to compute the checksum of the IP
> header part.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>
> Notes:
> v4:
> - fix payload length endianness
>
> v3:
> - function parameters provide tot_len, saddr, daddr and protocol
> rather than an iphdr/ipv6hdr
>
> v2:
> - move new function to checksum.c
> - use _psum rather than _checksum in the name
> - replace csum_udp4() and csum_udp6() by the new function
>
> checksum.c | 69 ++++++++++++++++++++++++++++++++++++++++++------------
> checksum.h | 4 ++++
> tcp.c | 45 ++++++++++++++++-------------------
> udp.c | 13 ++++++----
> 4 files changed, 86 insertions(+), 45 deletions(-)
>
> diff --git a/checksum.c b/checksum.c
> index 511b296a9a80..93c8d5205c2b 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -134,6 +134,30 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
> return ~csum_fold(sum);
> }
>
> +/**
> + * proto_ipv4_header_psum() - Calculates the partial checksum of an
> + * IPv4 header for UDP or TCP
> + * @tot_len: IPv4 Payload length
> + * @proto: Protocol number
> + * @saddr: Source address
> + * @daddr: Destination address
> + * @proto: proto Protocol number
Needs to note that tot_len is in host order, but saddr and daddr are
in network order. Usually, I'd take host order as assumed for a plain
integer type, but since it's mixed here, we should annotate them all.
Alternatively, we could pass saddr and daddr as struct in_addr. In
general I've tried to pass IPv4 addresses with that type, rather than
in_addr_t or uint32_t. Looking at the callers, it seems like it's a
mixed bag whether that's messier or cleaner in this case.
> + * Returns: Partial checksum of the IPv4 header
> + */
> +uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol,
> + uint32_t saddr, uint32_t daddr)
> +{
> + uint32_t psum = htons(protocol);
> +
> + psum += (saddr >> 16) & 0xffff;
> + psum += saddr & 0xffff;
> + psum += (daddr >> 16) & 0xffff;
> + psum += daddr & 0xffff;
> + psum += htons(tot_len);
> +
> + return psum;
> +}
> +
> /**
> * csum_udp4() - Calculate and set checksum for a UDP over IPv4 packet
> * @udp4hr: UDP header, initialised apart from checksum
> @@ -150,14 +174,12 @@ void csum_udp4(struct udphdr *udp4hr,
> 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(saddr.s_addr)
> - + csum_fold(daddr.s_addr)
> - + htons(len + sizeof(*udp4hr))
> - + htons(IPPROTO_UDP);
> - /* Add in partial checksum for the UDP header alone */
> - psum += sum_16b(udp4hr, sizeof(*udp4hr));
> + uint16_t tot_len = len + sizeof(struct udphdr);
> + uint32_t psum = proto_ipv4_header_psum(tot_len,
> + IPPROTO_UDP,
> + saddr.s_addr,
> + daddr.s_addr);
> + psum = csum_unfolded(udp4hr, sizeof(struct udphdr), psum);
> udp4hr->check = csum(payload, len, psum);
> }
> }
> @@ -180,6 +202,26 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len)
> icmp4hr->checksum = csum(payload, len, psum);
> }
>
> +/**
> + * proto_ipv6_header_psum() - Calculates the partial checksum of an
> + * IPv6 header for UDP or TCP
> + * @payload_len: IPv6 payload length
> + * @proto: Protocol number
> + * @saddr: Source address
> + * @daddr: Destination address
> + * Returns: Partial checksum of the IPv6 header
> + */
> +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
> + struct in6_addr saddr, struct in6_addr daddr)
I don't see any point to passing the addresses by value here. You
take their address, so they must be written back to memory if passed
in registers. At the call sites, you still have the dereference so it
doesn't help with alignment.
> +{
> + uint32_t sum = htons(protocol) + htons(payload_len);
> +
> + sum += sum_16b(&saddr, sizeof(saddr));
> + sum += sum_16b(&daddr, sizeof(daddr));
> +
> + return sum;
> +}
> +
> /**
> * csum_udp6() - Calculate and set checksum for a UDP over IPv6 packet
> * @udp6hr: UDP header, initialised apart from checksum
> @@ -190,14 +232,11 @@ void csum_udp6(struct udphdr *udp6hr,
> const struct in6_addr *saddr, const struct in6_addr *daddr,
> 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);
> -
> + uint32_t psum = proto_ipv6_header_psum(len + sizeof(struct udphdr),
> + IPPROTO_UDP, *saddr, *daddr);
> udp6hr->check = 0;
> - /* Add in partial checksum for the UDP header alone */
> - psum += sum_16b(udp6hr, sizeof(*udp6hr));
> +
> + psum = csum_unfolded(udp6hr, sizeof(struct udphdr), psum);
> udp6hr->check = csum(payload, len, psum);
> }
>
> diff --git a/checksum.h b/checksum.h
> index 92db73612b6e..b2b5b8e8b77e 100644
> --- a/checksum.h
> +++ b/checksum.h
> @@ -15,10 +15,14 @@ uint16_t csum_fold(uint32_t sum);
> uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init);
> uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol,
> uint32_t saddr, uint32_t daddr);
> +uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol,
> + uint32_t saddr, uint32_t daddr);
> void csum_udp4(struct udphdr *udp4hr,
> struct in_addr saddr, struct in_addr daddr,
> const void *payload, size_t len);
> void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len);
> +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
> + struct in6_addr saddr, struct in6_addr daddr);
> void csum_udp6(struct udphdr *udp6hr,
> const struct in6_addr *saddr, const struct in6_addr *daddr,
> const void *payload, size_t len);
> diff --git a/tcp.c b/tcp.c
> index ea0802c6b102..d78efa5401bb 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -939,39 +939,30 @@ static void tcp_sock_set_bufsize(const struct ctx *c, int s)
> * tcp_update_check_tcp4() - Update TCP checksum from stored one
> * @buf: L2 packet buffer with final IPv4 header
Function comment no longer matches the parameters.
> */
> -static void tcp_update_check_tcp4(struct tcp4_l2_buf_t *buf)
> +static void tcp_update_check_tcp4(struct iphdr *iph)
Hmm... so this takes only a pointer to iph, but writes to the TCP
header it assumes is beyond that, and reads from the payload it
assumes is beyond that. That seems like a dangerous interface to me
(not to mention that I fear it could trigger TBAA traps).
> {
> - uint16_t tlen = ntohs(buf->iph.tot_len) - 20;
> - uint32_t sum = htons(IPPROTO_TCP);
> + uint16_t tlen = ntohs(iph->tot_len) - sizeof(struct iphdr);
> + uint32_t sum = proto_ipv4_header_psum(tlen, IPPROTO_TCP,
> + iph->saddr, iph->daddr);
> + struct tcphdr *th = (struct tcphdr *)(iph + 1);
>
> - sum += (buf->iph.saddr >> 16) & 0xffff;
> - sum += buf->iph.saddr & 0xffff;
> - sum += (buf->iph.daddr >> 16) & 0xffff;
> - sum += buf->iph.daddr & 0xffff;
> - sum += htons(ntohs(buf->iph.tot_len) - 20);
> -
> - buf->th.check = 0;
> - buf->th.check = csum(&buf->th, tlen, sum);
> + th->check = 0;
> + th->check = csum(th, tlen, sum);
> }
>
> /**
> * tcp_update_check_tcp6() - Calculate TCP checksum for IPv6
> * @buf: L2 packet buffer with final IPv6 header
> */
> -static void tcp_update_check_tcp6(struct tcp6_l2_buf_t *buf)
> +static void tcp_update_check_tcp6(struct ipv6hdr *ip6h)
Same comments as for the IPv4 version.
> {
> - int len = ntohs(buf->ip6h.payload_len) + sizeof(struct ipv6hdr);
> -
> - buf->ip6h.hop_limit = IPPROTO_TCP;
> - buf->ip6h.version = 0;
> - buf->ip6h.nexthdr = 0;
> + struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
> + uint16_t payload_len = ntohs(ip6h->payload_len);
> + uint32_t sum = proto_ipv6_header_psum(payload_len, IPPROTO_TCP,
> + ip6h->saddr, ip6h->daddr);
>
> - buf->th.check = 0;
> - buf->th.check = csum(&buf->ip6h, len, 0);
> -
> - buf->ip6h.hop_limit = 255;
> - buf->ip6h.version = 6;
> - buf->ip6h.nexthdr = IPPROTO_TCP;
> + th->check = 0;
> + th->check = csum(th, payload_len, sum);
> }
>
> /**
> @@ -1383,7 +1374,7 @@ do { \
>
> SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>
> - tcp_update_check_tcp4(b);
> + tcp_update_check_tcp4(&b->iph);
>
> tlen = tap_iov_len(c, &b->taph, ip_len);
> } else {
> @@ -1402,7 +1393,11 @@ do { \
>
> SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
>
> - tcp_update_check_tcp6(b);
> + tcp_update_check_tcp6(&b->ip6h);
> +
> + b->ip6h.hop_limit = 255;
> + b->ip6h.version = 6;
> + b->ip6h.nexthdr = IPPROTO_TCP;
>
> b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
> b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
> diff --git a/udp.c b/udp.c
> index d517c99dcc69..410ace16a6a2 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -625,6 +625,7 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
> {
> struct udp6_l2_buf_t *b = &udp6_l2_buf[n];
> struct in6_addr *src;
> + uint16_t payload_len;
> in_port_t src_port;
> size_t ip_len;
>
> @@ -633,7 +634,8 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
>
> ip_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh);
>
> - b->ip6h.payload_len = htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh));
> + payload_len = udp6_l2_mh_sock[n].msg_len + sizeof(b->uh);
> + b->ip6h.payload_len = htons(payload_len);
>
> if (IN6_IS_ADDR_LINKLOCAL(src)) {
> b->ip6h.daddr = c->ip6.addr_ll_seen;
> @@ -675,10 +677,11 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
> b->uh.source = b->s_in6.sin6_port;
> b->uh.dest = htons(dstport);
> b->uh.len = b->ip6h.payload_len;
> -
> - b->ip6h.hop_limit = IPPROTO_UDP;
> - b->ip6h.version = b->ip6h.nexthdr = b->uh.check = 0;
> - b->uh.check = csum(&b->ip6h, ip_len, 0);
> + b->uh.check = 0;
> + b->uh.check = csum(&b->uh, payload_len,
> + proto_ipv6_header_psum(payload_len, IPPROTO_UDP,
> + b->ip6h.saddr,
> + b->ip6h.daddr));
> b->ip6h.version = 6;
> b->ip6h.nexthdr = IPPROTO_UDP;
> b->ip6h.hop_limit = 255;
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()
2024-02-29 16:59 ` [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
@ 2024-02-29 23:54 ` David Gibson
2024-03-03 13:50 ` Laurent Vivier
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2024-02-29 23:54 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 7713 bytes --]
On Thu, Feb 29, 2024 at 05:59:55PM +0100, Laurent Vivier wrote:
> Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function
> tcp_fill_header().
>
> Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to
> tcp_fill_ipv4_header() and tcp_fill_ipv6_header()
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>
> Notes:
> v4:
> - group all the ip6g initialisations together and
> remove flow_lbl preset to 0
> - add ASSERT(a4)
>
> v3:
> - add to sub-series part 1
>
> v2:
> - extract header filling functions from
> "tcp: extract buffer management from tcp_send_flag()"
> - rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(),
> tcp_fill_ipv6_header()
> - use upside-down Christmas tree arguments order
> - replace (void *) by (struct tcphdr *)
>
> tcp.c | 154 +++++++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 104 insertions(+), 50 deletions(-)
>
> diff --git a/tcp.c b/tcp.c
> index 5b2fdf662a6c..ced22534a103 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c)
> tcp_l2_data_buf_flush(c);
> }
>
> +/**
> + * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
> + *
> + * @th: Pointer to the TCP header structure
> + * @conn: Pointer to the TCP connection structure
> + * @seq: Sequence number
> + */
> +static void tcp_fill_header(struct tcphdr *th,
> + const struct tcp_tap_conn *conn, uint32_t seq)
> +{
> + th->source = htons(conn->fport);
> + th->dest = htons(conn->eport);
> + th->seq = htonl(seq);
> + th->ack_seq = htonl(conn->seq_ack_to_tap);
> + if (conn->events & ESTABLISHED) {
> + th->window = htons(conn->wnd_to_tap);
> + } else {
> + unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
> +
> + th->window = htons(MIN(wnd, USHRT_MAX));
> + }
> +}
> +
> +/**
> + * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
I don't love the name, since it does also fill the TCP header. Maybe
'tcp_fill_headers4()'?
> + * @c: Execution context
> + * @conn: Connection pointer
> + * @iph: Pointer to IPv4 header, immediately followed by a TCP header
Again, really don't love accessing beyond a given pointer's type.
> + * @plen: Payload length (including TCP header options)
> + * @check: Checksum, if already known
> + * @seq: Sequence number for this segment
> + *
> + * Return: IP frame length including L2 headers, host order
AFAICT the return value does *not* include the L2 headers..
> + */
> +static size_t tcp_fill_ipv4_header(const struct ctx *c,
> + const struct tcp_tap_conn *conn,
> + struct iphdr *iph, size_t plen,
> + const uint16_t *check, uint32_t seq)
> +{
> + size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> + const struct in_addr *a4 = inany_v4(&conn->faddr);
> + struct tcphdr *th = (struct tcphdr *)(iph + 1);
> +
> + ASSERT(a4);
> +
> + iph->tot_len = htons(ip_len);
> + iph->saddr = a4->s_addr;
> + iph->daddr = c->ip4.addr_seen.s_addr;
> +
> + iph->check = check ? *check :
> + csum_ip4_header(iph->tot_len, IPPROTO_TCP,
> + iph->saddr, iph->daddr);
> +
> +
> + tcp_fill_header(th, conn, seq);
> +
> + tcp_update_check_tcp4(iph);
It's a bit ugly that tcp_fill_header() fills the TCP header, but *not*
the checksum. Could we handle this by passing the pseudo-header psum
into tcp_fill_header()? Then the logic for that in
tcp_update_check_tcp4() would become part of this function.
> + return ip_len;
> +}
> +
> +/**
> + * tcp_fill_ipv6_header() - Fill 802.3, IPv6, TCP headers in pre-cooked buffers
> + * @c: Execution context
> + * @conn: Connection pointer
> + * @ip6h: Pointer to IPv6 header, immediately followed by a TCP header
> + * @plen: Payload length (including TCP header options)
> + * @check: Checksum, if already known
> + * @seq: Sequence number for this segment
> + *
> + * Return: The total length of the IPv6 packet, host order
> + */
> +static size_t tcp_fill_ipv6_header(const struct ctx *c,
> + const struct tcp_tap_conn *conn,
> + struct ipv6hdr *ip6h, size_t plen,
> + uint32_t seq)
> +{
> + size_t ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> + struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
> +
> + ip6h->payload_len = htons(plen + sizeof(struct tcphdr));
> + ip6h->saddr = conn->faddr.a6;
> + if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
> + ip6h->daddr = c->ip6.addr_ll_seen;
> + else
> + ip6h->daddr = c->ip6.addr_seen;
> +
> + ip6h->hop_limit = 255;
> + ip6h->version = 6;
> + ip6h->nexthdr = IPPROTO_TCP;
> +
> + ip6h->flow_lbl[0] = (conn->sock >> 16) & 0xf;
> + ip6h->flow_lbl[1] = (conn->sock >> 8) & 0xff;
> + ip6h->flow_lbl[2] = (conn->sock >> 0) & 0xff;
> +
> + tcp_fill_header(th, conn, seq);
> +
> + tcp_update_check_tcp6(ip6h);
> +
> + return ip_len;
> +}
> +
> /**
> * tcp_l2_buf_fill_headers() - Fill 802.3, IP, TCP headers in pre-cooked buffers
> * @c: Execution context
> @@ -1345,67 +1447,19 @@ static size_t tcp_l2_buf_fill_headers(const struct ctx *c,
> const struct in_addr *a4 = inany_v4(&conn->faddr);
> size_t ip_len, tlen;
>
> -#define SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq) \
> -do { \
> - b->th.source = htons(conn->fport); \
> - b->th.dest = htons(conn->eport); \
> - b->th.seq = htonl(seq); \
> - b->th.ack_seq = htonl(conn->seq_ack_to_tap); \
> - if (conn->events & ESTABLISHED) { \
> - b->th.window = htons(conn->wnd_to_tap); \
> - } else { \
> - unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap; \
> - \
> - b->th.window = htons(MIN(wnd, USHRT_MAX)); \
> - } \
> -} while (0)
> -
> if (a4) {
> struct tcp4_l2_buf_t *b = (struct tcp4_l2_buf_t *)p;
>
> - ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> - b->iph.tot_len = htons(ip_len);
> - b->iph.saddr = a4->s_addr;
> - b->iph.daddr = c->ip4.addr_seen.s_addr;
> -
> - b->iph.check = check ? *check :
> - csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
> - b->iph.saddr, b->iph.daddr);
> -
> - SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> - tcp_update_check_tcp4(&b->iph);
> + ip_len = tcp_fill_ipv4_header(c, conn, &b->iph, plen, check, seq);
>
> tlen = tap_iov_len(c, &b->taph, ip_len);
> } else {
> struct tcp6_l2_buf_t *b = (struct tcp6_l2_buf_t *)p;
>
> - ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> -
> - b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
> - b->ip6h.saddr = conn->faddr.a6;
> - if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
> - b->ip6h.daddr = c->ip6.addr_ll_seen;
> - else
> - b->ip6h.daddr = c->ip6.addr_seen;
> -
> - memset(b->ip6h.flow_lbl, 0, 3);
> -
> - SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> - tcp_update_check_tcp6(&b->ip6h);
> -
> - b->ip6h.hop_limit = 255;
> - b->ip6h.version = 6;
> - b->ip6h.nexthdr = IPPROTO_TCP;
> -
> - b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
> - b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
> - b->ip6h.flow_lbl[2] = (conn->sock >> 0) & 0xff;
> + ip_len = tcp_fill_ipv6_header(c, conn, &b->ip6h, plen, seq);
>
> tlen = tap_iov_len(c, &b->taph, ip_len);
> }
> -#undef SET_TCP_HEADER_COMMON_V4_V6
>
> return tlen;
> }
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch]
2024-02-29 23:19 ` David Gibson
@ 2024-03-01 12:06 ` Laurent Vivier
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-03-01 12:06 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On 3/1/24 00:19, David Gibson wrote:
> On Thu, Feb 29, 2024 at 05:59:51PM +0100, Laurent Vivier wrote:
>> Introduce ip.[ch] file to encapsulate IP protocol handling functions and
>> structures. Modify various files to include the new header ip.h when
>> it's needed.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>
>> Notes:
>> v4:
>> - rebase
>>
>> v3:
>> - rewrap rationale
>> - add David's R-b
>>
>> v2:
>> - update rationale and comments
>>
>> Makefile | 10 +++---
>> conf.c | 1 +
>> dhcp.c | 1 +
>> flow.c | 1 +
>> fwd.c | 1 +
>> icmp.c | 1 +
>> inany.c | 1 +
>> ip.c | 72 +++++++++++++++++++++++++++++++++++++++++++
>> ip.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> Noe that ip.h exists, in4addr_loopback and in4addr_any should probably
> go in there rather than inany.h. That can be a followup change
> though.
>
Not sure they should be moved to ip.h: they depend on inany_loopback4 and inany_any4 that
are both defined in inany.c and the structure inany_addr is defined in inany.h
I think it's better to let them where they are.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c
2024-02-29 23:25 ` David Gibson
@ 2024-03-01 12:31 ` Laurent Vivier
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-03-01 12:31 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On 3/1/24 00:25, David Gibson wrote:
> On Thu, Feb 29, 2024 at 05:59:52PM +0100, Laurent Vivier wrote:
>> We can find the same function to compute the IPv4 header
>> checksum in tcp.c, udp.c and tap.c
>>
>> Use the function defined for tap.c, csum_ip4_header(), but
>> with the code used in tcp.c and udp.c as it doesn't need a fully
>> initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>
>> Notes:
>> v4:
>> - rebase
>>
>> v3:
>> - function parameters provide tot_len, saddr, daddr and protocol
>> rather than an iphdr
>>
>> v2:
>> - use csum_ip4_header() from checksum.c
>> - use code from tcp.c and udp.c in csum_ip4_header()
>> - use "const struct iphfr *", check is not updated by the
>> function but by the caller.
>>
>> checksum.c | 17 +++++++++++++----
>> checksum.h | 3 ++-
>> tap.c | 3 ++-
>> tcp.c | 24 +++---------------------
>> udp.c | 20 ++------------------
>> 5 files changed, 22 insertions(+), 45 deletions(-)
>>
>> diff --git a/checksum.c b/checksum.c
>> index 74e3742bc6f6..511b296a9a80 100644
>> --- a/checksum.c
>> +++ b/checksum.c
>> @@ -57,6 +57,7 @@
>> #include <linux/icmpv6.h>
>>
>> #include "util.h"
>> +#include "ip.h"
>> #include "checksum.h"
>>
>> /* Checksums are optional for UDP over IPv4, so we usually just set
>> @@ -116,13 +117,21 @@ uint16_t csum_fold(uint32_t sum)
>> uint16_t csum(const void *buf, size_t len, uint32_t init);
>>
>> /**
>> - * csum_ip4_header() - Calculate and set IPv4 header checksum
>> + * csum_ip4_header() - Calculate IPv4 header checksum
>> * @ip4h: IPv4 header
>
> Function comment needs to be updated for the new parameters. In
> particular it needs to note that tot_len, saddr and daddr are all
> passed in network order.
>
> As noted elsewhere, I kind of hate passing non-host-endian values in
> plain integer types, but I can see why doing otherwise here would be
> very awkward.
Perhaps we can use __be16 and __be32 types to really show the endianness in the code?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()
2024-02-29 23:54 ` David Gibson
@ 2024-03-03 13:50 ` Laurent Vivier
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Vivier @ 2024-03-03 13:50 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On 3/1/24 00:54, David Gibson wrote:
> On Thu, Feb 29, 2024 at 05:59:55PM +0100, Laurent Vivier wrote:
>> Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function
>> tcp_fill_header().
>>
>> Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to
>> tcp_fill_ipv4_header() and tcp_fill_ipv6_header()
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>
>> Notes:
>> v4:
>> - group all the ip6g initialisations together and
>> remove flow_lbl preset to 0
>> - add ASSERT(a4)
>>
>> v3:
>> - add to sub-series part 1
>>
>> v2:
>> - extract header filling functions from
>> "tcp: extract buffer management from tcp_send_flag()"
>> - rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(),
>> tcp_fill_ipv6_header()
>> - use upside-down Christmas tree arguments order
>> - replace (void *) by (struct tcphdr *)
>>
>> tcp.c | 154 +++++++++++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 104 insertions(+), 50 deletions(-)
>>
>> diff --git a/tcp.c b/tcp.c
>> index 5b2fdf662a6c..ced22534a103 100644
>> --- a/tcp.c
>> +++ b/tcp.c
>> @@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c)
>> tcp_l2_data_buf_flush(c);
>> }
>>
>> +/**
>> + * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
>> + *
>> + * @th: Pointer to the TCP header structure
>> + * @conn: Pointer to the TCP connection structure
>> + * @seq: Sequence number
>> + */
>> +static void tcp_fill_header(struct tcphdr *th,
>> + const struct tcp_tap_conn *conn, uint32_t seq)
>> +{
>> + th->source = htons(conn->fport);
>> + th->dest = htons(conn->eport);
>> + th->seq = htonl(seq);
>> + th->ack_seq = htonl(conn->seq_ack_to_tap);
>> + if (conn->events & ESTABLISHED) {
>> + th->window = htons(conn->wnd_to_tap);
>> + } else {
>> + unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
>> +
>> + th->window = htons(MIN(wnd, USHRT_MAX));
>> + }
>> +}
>> +
>> +/**
>> + * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
>
> I don't love the name, since it does also fill the TCP header. Maybe
> 'tcp_fill_headers4()'?
>
>> + * @c: Execution context
>> + * @conn: Connection pointer
>> + * @iph: Pointer to IPv4 header, immediately followed by a TCP header
>
> Again, really don't love accessing beyond a given pointer's type.
>
>> + * @plen: Payload length (including TCP header options)
>> + * @check: Checksum, if already known
>> + * @seq: Sequence number for this segment
>> + *
>> + * Return: IP frame length including L2 headers, host order
>
> AFAICT the return value does *not* include the L2 headers..
>
>> + */
>> +static size_t tcp_fill_ipv4_header(const struct ctx *c,
>> + const struct tcp_tap_conn *conn,
>> + struct iphdr *iph, size_t plen,
>> + const uint16_t *check, uint32_t seq)
>> +{
>> + size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
>> + const struct in_addr *a4 = inany_v4(&conn->faddr);
>> + struct tcphdr *th = (struct tcphdr *)(iph + 1);
>> +
>> + ASSERT(a4);
>> +
>> + iph->tot_len = htons(ip_len);
>> + iph->saddr = a4->s_addr;
>> + iph->daddr = c->ip4.addr_seen.s_addr;
>> +
>> + iph->check = check ? *check :
>> + csum_ip4_header(iph->tot_len, IPPROTO_TCP,
>> + iph->saddr, iph->daddr);
>> +
>> +
>> + tcp_fill_header(th, conn, seq);
>> +
>> + tcp_update_check_tcp4(iph);
>
> It's a bit ugly that tcp_fill_header() fills the TCP header, but *not*
> the checksum. Could we handle this by passing the pseudo-header psum
> into tcp_fill_header()? Then the logic for that in
> tcp_update_check_tcp4() would become part of this function.
The problem with that is we must also pass the payload (that is after the TCP header) and
the payload length. So we need to add two parameters to tcp_fill_header(); psum and
payload_length (guessing also the payload is following "th").
Moreover in the vhost-user part tcp_update_check_tcp4() is now called conditionally,
because the checksum is computed in the vhost-code as the payload is stripped along
several iovecs.
I'm going to send my v5 without updating this part. If you really think it should be done
differently please give me more details (considering also the vhost-user part).
Thanks,
Laurent
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-03-03 13:50 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
2024-02-29 23:16 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 2/8] checksum: align buffers Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 3/8] checksum: add csum_iov() Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
2024-02-29 23:19 ` David Gibson
2024-03-01 12:06 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
2024-02-29 23:25 ` David Gibson
2024-03-01 12:31 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
2024-02-29 23:42 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 7/8] tap: make tap_update_mac() generic Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
2024-02-29 23:54 ` David Gibson
2024-03-03 13:50 ` Laurent Vivier
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).