From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 74E885A027B for ; Fri, 16 Feb 2024 10:08:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708074527; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=odUnIgkCUhmaNmUf4K0uI78TBcatxHM3783zqUEOve0=; b=MSAQvJHXutP7Tk5qq+Xmm9Nk8khkvT8VGiVSok3pVMzzSG7ZsZdsX9zAR9rp7pp3tJPby4 aq0wzJQKic/Qnvzf9qqLg2HIKc1t0yWZ4I9Cmr5VJNH9OOyuSAnXx/DgXYY6lipyeV0meH KuS7/xCw2zhfdHypD42+AoZ5LMBtry8= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-458-F5lR-yFvM5ut0m_p1OXlWg-1; Fri, 16 Feb 2024 04:08:45 -0500 X-MC-Unique: F5lR-yFvM5ut0m_p1OXlWg-1 Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-a3158fbb375so31979966b.3 for ; Fri, 16 Feb 2024 01:08:45 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708074524; x=1708679324; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=odUnIgkCUhmaNmUf4K0uI78TBcatxHM3783zqUEOve0=; b=rr+O4T/EkNM7khY8oZpKebcpiwU+vjoIf4SP8l4Mvs1oPN1lBwvohCH5YpWuKbAxoK NWEkSJupuaJEI1cQvnlu1Fplz5k0tFBrEPqLnbduUE3cENjBBImfqlViAKvk81zum5lh I9VH2XUKhk5n9Zu2ZBnGfJSEoRKgCaBVe9oogZapHWVgoRbWhMXnQq5/EMsMkfcsq7vI DV6yF+0oAHpR2dnJRX8iHNoxFgjy6rwECgPlJZiok7vR5tbt398+7CGzdcyWP01gEoXm SHiD9yifRBoJcRLAjXqyLd4/1XlUx5/iJjxCilrLTPiqUGTL9kC2wQni8st/dzI6w1X4 g3zA== X-Gm-Message-State: AOJu0Yw1jkfRf0H9fZkUS2f0zYXM/0cCzIMmefcHibzfFh6MMxAzh/OO fziKPGxkgCTNhpSZMOQMAmuiwbZla8ZyoYJjpZuSe/XLWqp7yvMjjzMUHjg+2Q2vzNdy0WEa152 TINlBsE8//jyqtRgTJ45Wpf8HvcKiyNUmjRHwnYi6+sqNaiH+FakZ6XpRuvFqiDJCjC4Hr56LcC 6LjVk/NQfaqbRFMmK1j0sGKUNNomAAf7WmjL4= X-Received: by 2002:a17:906:578d:b0:a3d:98a9:1c73 with SMTP id k13-20020a170906578d00b00a3d98a91c73mr3149416ejq.55.1708074524052; Fri, 16 Feb 2024 01:08:44 -0800 (PST) X-Google-Smtp-Source: AGHT+IGbFeaWEH5JH2ln/TdWL21YQDd3tbiM/G5cEQAOr8xMTBDK/t3ssm4nKsq/sqM8ioIcc1TmHQ== X-Received: by 2002:a17:906:578d:b0:a3d:98a9:1c73 with SMTP id k13-20020a170906578d00b00a3d98a91c73mr3149404ejq.55.1708074523589; Fri, 16 Feb 2024 01:08:43 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id vg12-20020a170907d30c00b00a3d6a2d3904sm1364149ejc.146.2024.02.16.01.08.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Feb 2024 01:08:42 -0800 (PST) Date: Fri, 16 Feb 2024 10:08:05 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c Message-ID: <20240216100805.040826b3@elisabeth> In-Reply-To: <20240214085628.210783-7-lvivier@redhat.com> References: <20240214085628.210783-1-lvivier@redhat.com> <20240214085628.210783-7-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: R7OWI6DMYJ2OUZRQZ5KQDZSA7E64LKSG X-Message-ID-Hash: R7OWI6DMYJ2OUZRQZ5KQDZSA7E64LKSG X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 14 Feb 2024 09:56:26 +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 > --- > > Notes: > 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 | 16 ++++++++++++---- > checksum.h | 2 +- > tap.c | 2 +- > tcp.c | 22 +--------------------- > udp.c | 23 +++++------------------ > 5 files changed, 20 insertions(+), 45 deletions(-) > > diff --git a/checksum.c b/checksum.c > index ac2bc49f7eb0..5613187a1c82 100644 > --- a/checksum.c > +++ b/checksum.c > @@ -57,6 +57,7 @@ > #include > > #include "util.h" > +#include "ip.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 > @@ -115,13 +116,20 @@ 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(const struct iphdr *ip4h) > { > - ip4h->check = 0; > - ip4h->check = csum(ip4h, (size_t)ip4h->ihl * 4, 0); > + uint32_t sum = L2_BUF_IP4_PSUM(ip4h->protocol); > + > + sum += ip4h->tot_len; > + sum += (ip4h->saddr >> 16) & 0xffff; > + sum += ip4h->saddr & 0xffff; > + sum += (ip4h->daddr >> 16) & 0xffff; > + sum += ip4h->daddr & 0xffff; > + > + return ~csum_fold(sum); > } > > /** > diff --git a/checksum.h b/checksum.h > index 6a20297a5826..b87ecd720df5 100644 > --- a/checksum.h > +++ b/checksum.h > @@ -13,7 +13,7 @@ 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(const struct iphdr *ip4h); > 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 3ea03f720d6d..70f36a55314f 100644 > --- a/tap.c > +++ b/tap.c > @@ -160,7 +160,7 @@ 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); > return ip4h + 1; > } > > diff --git a/tcp.c b/tcp.c > index 45ef5146729a..35e240f4ffc3 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -934,23 +934,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 > @@ -1393,10 +1376,7 @@ 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); > > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > > diff --git a/udp.c b/udp.c > index d514c864ab5b..e645c800a823 100644 > --- a/udp.c > +++ b/udp.c > @@ -270,23 +270,6 @@ static void udp_invert_portmap(struct udp_port_fwd *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 > @@ -579,6 +562,9 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n, > * > * Return: size of tap frame with headers > */ > +#pragma GCC diagnostic push > +/* ignore unaligned pointer value warning for &b->iph */ > +#pragma GCC diagnostic ignored "-Waddress-of-packed-member" > static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport, > const struct timespec *now) > { > @@ -614,13 +600,14 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport, > b->iph.saddr = b->s_in.sin_addr.s_addr; > } > > - udp_update_check4(b); > + b->iph.check = csum_ip4_header(&b->iph); Similar comment as I had on v1: I don't think this is safe. If &b->iph is, say, 0x2000, it's all fine: when csum_ip4_header() needs to access, say, ip4h->tot_len, it will dereference 0x2000 and look at 16 bits, 2 bytes into it. If &b->iph is 0x2001, though, csum_ip4_header() will dereference 0x2001 and, on some architectures, boom. You need to pass b, or, if possible, to align iph to a 4-bytes boundary. There's a reason why I implemented it like it is now. The current version is rather inconvenient and ugly, so it's great if you manage to improve it this way, but you shouldn't risk dereferencing unaligned pointers... unless you know for some reason that they are aligned, of course. -- Stefano