Re: [PATCH 1/7] tcp: pass ipv4h checksum, not a pointer to the checksum

On 3/24/26 04:53, David Gibson wrote:

On Mon, Mar 23, 2026 at 05:52:53PM +0100, Laurent Vivier wrote:

...

tcp_fill_headers() takes a pointer to a previously computed IPv4 header checksum to avoid recalculating it when the payload length doesn't change. A subsequent patch makes tcp_fill_headers() access ip4h via with_header() which scopes it to a temporary variable, so a pointer to ip4h->check would become dangling after the with_header() block.

Oof, that kind of indicates the dangers with the with_header() structure. Is that change merged already? If not we should probably fix it before merge rather than after the fact.

The problem appears in the subsequent patches of the series. So at this point we prevent the problem, don't fix it.

...

Pass the checksum by value as an int instead, using -1 as the sentinel to indicate that the checksum should be computed from scratch (replacing the NULL pointer sentinel). As the checksum is a uint16_t, -1 cannot be a valid checksum value in an int.

That said, passing this by value I think is cleaner than the pointer, regardless of other reasons. Would it also make sense to flag no_tcp_csum with an additional special value (a #define to -2, or whatever) instead of using an extra parameter? Logically the checksum parameter would be: CALCULATE | UNNEEDED | specific value

I can extract the patch from this series to make this improvement on the current code. Thanks, Laurent

Reviewed-by: David Gibson

...

Signed-off-by: Laurent Vivier --- tcp.c | 7 +++---- tcp_buf.c | 10 +++++----- tcp_internal.h | 3 +-- tcp_vu.c | 12 ++++++------ 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/tcp.c b/tcp.c index b14586249c4e..158a5be0327e 100644 --- a/tcp.c +++ b/tcp.c @@ -953,8 +953,7 @@ size_t tcp_fill_headers(const struct ctx *c, struct tcp_tap_conn *conn, struct ethhdr *eh, struct iphdr *ip4h, struct ipv6hdr *ip6h, struct tcphdr *th, struct iov_tail *payload, - const uint16_t *ip4_check, uint32_t seq, - bool no_tcp_csum) + int ip4_check, uint32_t seq, bool no_tcp_csum) { const struct flowside *tapside = TAPFLOW(conn); size_t l4len = iov_tail_size(payload) + sizeof(*th); @@ -974,8 +973,8 @@ size_t tcp_fill_headers(const struct ctx *c, struct tcp_tap_conn *conn, ip4h->saddr = src4->s_addr; ip4h->daddr = dst4->s_addr;

- if (ip4_check) - ip4h->check = *ip4_check; + if (ip4_check != -1) + ip4h->check = ip4_check; else ip4h->check = csum_ip4_header(l3len, IPPROTO_TCP, *src4, *dst4); diff --git a/tcp_buf.c b/tcp_buf.c index 41965b107567..bc0f58dd7a5e 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -172,7 +172,7 @@ static void tcp_l2_buf_pad(struct iovec *iov) */ static void tcp_l2_buf_fill_headers(const struct ctx *c, struct tcp_tap_conn *conn, - struct iovec *iov, const uint16_t *check, + struct iovec *iov, int check, uint32_t seq, bool no_tcp_csum) { struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); @@ -233,7 +233,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) if (flags & KEEPALIVE) seq--;

- tcp_l2_buf_fill_headers(c, conn, iov, NULL, seq, false); + tcp_l2_buf_fill_headers(c, conn, iov, -1, seq, false);

tcp_l2_buf_pad(iov);

@@ -270,7 +270,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, ssize_t dlen, int no_csum, uint32_t seq, bool push) { struct tcp_payload_t *payload; - const uint16_t *check = NULL; + int check = -1; struct iovec *iov;

conn->seq_to_tap = seq + dlen; @@ -279,9 +279,9 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, if (CONN_V4(conn)) { if (no_csum) { struct iovec *iov_prev = tcp_l2_iov[tcp_payload_used - 1]; - struct iphdr *iph = iov_prev[TCP_IOV_IP].iov_base; + const struct iphdr *iph = iov_prev[TCP_IOV_IP].iov_base;

- check = &iph->check; + check = iph->check; } iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_used]); } else if (CONN_V6(conn)) { diff --git a/tcp_internal.h b/tcp_internal.h index d9408852571f..bb7a6629839c 100644 --- a/tcp_internal.h +++ b/tcp_internal.h @@ -187,8 +187,7 @@ size_t tcp_fill_headers(const struct ctx *c, struct tcp_tap_conn *conn, struct ethhdr *eh, struct iphdr *ip4h, struct ipv6hdr *ip6h, struct tcphdr *th, struct iov_tail *payload, - const uint16_t *ip4_check, uint32_t seq, - bool no_tcp_csum); + int ip4_check, uint32_t seq, bool no_tcp_csum);

int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn, bool force_seq, struct tcp_info_linux *tinfo); diff --git a/tcp_vu.c b/tcp_vu.c index 3001defb5467..a21ee3499aed 100644 --- a/tcp_vu.c +++ b/tcp_vu.c @@ -138,7 +138,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) seq--;

tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload, - NULL, seq, !*c->pcap); + -1, seq, !*c->pcap);

if (*c->pcap) pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN); @@ -283,7 +283,7 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, */ static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *conn, struct iovec *iov, size_t iov_cnt, - const uint16_t **check, bool no_tcp_csum, bool push) + int *check, bool no_tcp_csum, bool push) { const struct flowside *toside = TAPFLOW(conn); bool v6 = !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)); @@ -329,7 +329,7 @@ static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *conn, tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload, *check, conn->seq_to_tap, no_tcp_csum); if (ip4h) - *check = &ip4h->check; + *check = ip4h->check; }

/** @@ -350,7 +350,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) size_t hdrlen, fillsize; int v6 = CONN_V6(conn); uint32_t already_sent; - const uint16_t *check; + int check;

if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) { debug("Got packet, but RX virtqueue not usable yet"); @@ -437,7 +437,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) */

hdrlen = tcp_vu_hdrlen(v6); - for (i = 0, previous_dlen = -1, check = NULL; i < head_cnt; i++) { + for (i = 0, previous_dlen = -1, check = -1; i < head_cnt; i++) { struct iovec *iov = &elem[head[i]].in_sg[0]; int buf_cnt = head[i + 1] - head[i]; size_t frame_size = iov_size(iov, buf_cnt); @@ -451,7 +451,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)

/* The IPv4 header checksum varies only with dlen */ if (previous_dlen != dlen) - check = NULL; + check = -1; previous_dlen = dlen;

tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap, push); -- 2.53.0