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.129.124]) by passt.top (Postfix) with ESMTP id 6AC4E5A026D for ; Fri, 8 Mar 2024 01:09:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709856564; 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=VT0zUhuoAHnyjOhRzJNX26DJCwuyZJGNCA0xS6Ylqcs=; b=Mct8azLXd7Xz1M/RiLIdksEn0n9RbI16dHEjw8txF326l3hlHgUCl53kV5eWKj93e5Gv+A rPS90yJHKZYWs9Wg5K0+shCvXPJkoWg8dMg3eANC08PlxIiLacmE0ZAm/7ZjqYk2NHWW45 lJX/CjpEhuvDBHJmVAsA9tLS1WOtoGo= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-39-FXk8jpZpM1iteiRbJx8b5w-1; Thu, 07 Mar 2024 19:09:22 -0500 X-MC-Unique: FXk8jpZpM1iteiRbJx8b5w-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-a45c9d4c216so77958366b.0 for ; Thu, 07 Mar 2024 16:09:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709856561; x=1710461361; 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=lZPu7QyuZmgIsoTRU3XQ+oAYdZLGs9o+hY+Uk6Rg3Fc=; b=mNY4iWbVEsC1mYR7dHcFhCWcXX2rpZmT9XhWiDDqAdw3v4rbmV4d3oy2M/Bd5K4rpB 3PRAaMz+TvINiB+SHuzhda7LSip82hkGzgl/QDVpHI2+FDhq4gC55KSsHZDdoZmbfVeo 6XKLjA6HJncXpxhnjjjT6bSldGVmSjX2yjsFqm3JAKvgwOGIrTL8MZSxdfRp0hLYGh9I A+3rwkufMkE807KWWMy/FyCRsFIU89OhlQpPeAl0wFcX7QlriHOzsu88Ukcfyjl0V/qM aUQ6hOxRa8EpFCqjCCeevutNO7Xuai/E7qhzlJdoXs7//rxoplBQMZAkpTbV3GsMvh5m vGZg== X-Forwarded-Encrypted: i=1; AJvYcCWVb5O1pk6TOatbFiF7aUWZTLhcKb8QHVImiRkUAnwvyUy/L7MlX1JihMr59yk5hlD4zxOFOwJgSzlcD9kdZMOyAovE X-Gm-Message-State: AOJu0YwtO11YsNDgTOsSexusFa3O/b7t07crQdS/Nx926ZCqULb54fTz Z3tXDHf6jM/rGgzF2tDTgbqsNkO6uAHhgY/fYbYcY5omEHSJLul/UFc/xkHadZ7wiS6gzR4XWuS hAEjrlmCl6BQ9L5zMZQb3dEXaKO+K250w78Glp9G2I0TultBDiA== X-Received: by 2002:a17:906:270d:b0:a45:ab75:7628 with SMTP id z13-20020a170906270d00b00a45ab757628mr6622765ejc.52.1709856561003; Thu, 07 Mar 2024 16:09:21 -0800 (PST) X-Google-Smtp-Source: AGHT+IGV9SpplBAngb7YY8pQogjFSaNfRBXYfBd4XLbJcRhyTtA2QxqE7DKLrebZZFA2a+0rkg6kyw== X-Received: by 2002:a17:906:270d:b0:a45:ab75:7628 with SMTP id z13-20020a170906270d00b00a45ab757628mr6622755ejc.52.1709856560411; Thu, 07 Mar 2024 16:09:20 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id gl7-20020a170906e0c700b00a44e65447f9sm6425881ejb.7.2024.03.07.16.09.19 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Mar 2024 16:09:19 -0800 (PST) Date: Fri, 8 Mar 2024 01:08:45 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 7/9] checksum: introduce functions to compute the header part checksum for TCP/UDP Message-ID: <20240308010845.4a15b5a5@elisabeth> In-Reply-To: References: <20240229080509.4f534831@elisabeth> <20240229095625.557367ab@elisabeth> <20240229151553.60d5cf18@elisabeth> <20240301075651.42ec7145@elisabeth> <20240304120040.1cebc230@elisabeth> <20240304234717.5f697efd@elisabeth> 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=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 6ZDRVG7FALTNAGRU7HZHXPX5RFNMAIWY X-Message-ID-Hash: 6ZDRVG7FALTNAGRU7HZHXPX5RFNMAIWY 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: Laurent Vivier , 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, 6 Mar 2024 16:09:23 +1100 David Gibson wrote: > On Mon, Mar 04, 2024 at 11:47:17PM +0100, Stefano Brivio wrote: > > On Mon, 4 Mar 2024 12:00:40 +0100 > > Stefano Brivio wrote: > > =20 > > > On Mon, 4 Mar 2024 12:54:12 +1100 > > > David Gibson wrote: > > > =20 > > > > On Fri, Mar 01, 2024 at 07:56:51AM +0100, Stefano Brivio wrote: = =20 > > > > > On Fri, 1 Mar 2024 10:09:39 +1100 > > > > > David Gibson wrote: > > > > > =20 > > > > > > On Thu, Feb 29, 2024 at 03:15:53PM +0100, Stefano Brivio wrote:= =20 > > > > > > > On Thu, 29 Feb 2024 09:56:25 +0100 > > > > > > > Stefano Brivio wrote: > > > > > > > =20 > > > > > > > > On Thu, 29 Feb 2024 19:49:09 +1100 > > > > > > > > David Gibson wrote: > > > > > > > > =20 > > > > > > > > > On Thu, Feb 29, 2024 at 08:05:09AM +0100, Stefano Brivio = wrote: =20 > > > > > > > > > > On Thu, 29 Feb 2024 11:38:53 +1100 > > > > > > > > > > David Gibson wrote: > > > > > > > > > > =20 > > > > > > > > > > > On Wed, Feb 28, 2024 at 02:26:18PM +0100, Laurent Viv= ier wrote: =20 > > > > > > > > > > > > On 2/19/24 04:08, David Gibson wrote: = =20 > > > > > > > > > > > > > On Sat, Feb 17, 2024 at 04:07:23PM +0100, Laurent= Vivier wrote: =20 > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > =20 > > > > > > > > > > > > > > +/** > > > > > > > > > > > > > > + * proto_ipv6_header_psum() - Calculates the p= artial checksum of an > > > > > > > > > > > > > > + * =09=09=09 IPv6 header for UDP or TCP > > > > > > > > > > > > > > + * @payload_len:=09Payload length > > > > > > > > > > > > > > + * @proto:=09=09Protocol number > > > > > > > > > > > > > > + * @saddr:=09=09Source address > > > > > > > > > > > > > > + * @daddr:=09=09Destination address > > > > > > > > > > > > > > + * Returns:=09Partial checksum of the IPv6 hea= der > > > > > > > > > > > > > > + */ > > > > > > > > > > > > > > +uint32_t proto_ipv6_header_psum(uint16_t paylo= ad_len, uint8_t protocol, > > > > > > > > > > > > > > +=09=09=09=09struct in6_addr saddr, struct in6_= addr daddr) =20 > > > > > > > > > > > > >=20 > > > > > > > > > > > > > Hrm, this is passing 2 16-byte IPv6 addresses by = value, which might > > > > > > > > > > > > > not be what we want. =20 > > > > > > > > > > > >=20 > > > > > > > > > > > > The idea here is to avoid the pointer alignment pro= blem (&ip6h->saddr and > > > > > > > > > > > > &ip6h->daddr can be misaligned). =20 > > > > > > > > > > >=20 > > > > > > > > > > > Ah, right. That's a neat idea, but I'm not sure it r= eally helps: I > > > > > > > > > > > think it will just move the misaligned access from in= side the function > > > > > > > > > > > to the call site, where we try to marshal the paramet= er from something > > > > > > > > > > > unaligned. =20 > > > > > > > > > >=20 > > > > > > > > > > I haven't tested this yet, but note that this is genera= lly okay: the > > > > > > > > > > problem is *dereferencing* an unaligned pointer. But if= you load memory > > > > > > > > > > from an aligned pointer, and extract a value from this = memory, it's all > > > > > > > > > > fine. =20 > > > > > > > > >=20 > > > > > > > > > Right, that's kind of what I'm getting at. Assuming this= value starts > > > > > > > > > in an unaligned buffer, then in order to pass this by val= ue the caller > > > > > > > > > will need to load from that unaligned pointer. AFAIK, th= e compiler > > > > > > > > > will base the type of loads only on the pointed to type, = which isn't > > > > > > > > > changed whether we dereference in the caller or the calle= e. > > > > > > > > > =20 > > > > > > > > > >=20 > > > > > > > > > > Speaking MIPS, this is not safe on all CPU models: > > > > > > > > > >=20 > > > > > > > > > > =09la=09$1, 1002 # s1 now contains the value 1002 > > > > > > > > > > =09lw=09$2, 0($1) # load word from memory at 1002 + 0 = into s2 > > > > > > > > > >=20 > > > > > > > > > > but this is: > > > > > > > > > >=20 > > > > > > > > > > =09la=09$1, 1000 # s1 now contains the value 1000 > > > > > > > > > > =09la=09$2, 1004 # s3 now contains the value 1004 > > > > > > > > > > =09lw=09$3, 0($1) # load word from memory at 1000 + 0 = into s3 > > > > > > > > > > =09lw=09$4, 0($3) # load word from memory at 1004 + 0 = into s4 > > > > > > > > > > =09sll=09$5, $3, 16 # 16-bit shift left s3 into s5 > > > > > > > > > > =09srl=09$6, $4, 16 # 16-bit shift right s4 into s6 > > > > > > > > > > =09or=09$2, $5, $6 # OR s5 and s6 into s2 =20 > > > > > > > > >=20 > > > > > > > > > Right, but I don't think merely moving the dereference to= the caller > > > > > > > > > will necessarily induce the compiler to generate this rat= her than the > > > > > > > > > former. =20 > > > > > > > >=20 > > > > > > > > Oh, oops, I didn't realise this was the case (I haven't rev= iewed the > > > > > > > > patch yet). =20 > > > > > > >=20 > > > > > > > ...no, that's not the case. Dereferencing 'iph' from > > > > > > > struct tcp[46]_l2_buf_t is fine: > > > > > > >=20 > > > > > > > struct tcp4_l2_buf_t { > > > > > > > uint8_t pad[2]; /* = 0 2 */ > > > > > > > struct tap_hdr taph; /* = 2 18 */ > > > > > > > struct iphdr iph; /* = 20 20 */ > > > > > > > =09[...] > > > > > > > } __attribute__((__packed__)); > > > > > > >=20 > > > > > > > struct tcp6_l2_buf_t { > > > > > > > uint8_t pad[2]; /* = 0 2 */ > > > > > > > struct tap_hdr taph; /* = 2 18 */ > > > > > > > struct ipv6hdr ip6h; /* = 20 40 */ > > > > > > > =09[...] > > > > > > > } __attribute__((__packed__)); > > > > > > >=20 > > > > > > > The problematic structures are the UDP buffers: > > > > > > >=20 > > > > > > > struct udp4_l2_buf_t { > > > > > > > struct sockaddr_in s_in; /* = 0 16 */ > > > > > > > struct tap_hdr taph; /* = 16 18 */ > > > > > > > struct iphdr iph; /* = 34 20 */ > > > > > > > =09[...] > > > > > > > } __attribute__((__aligned__(4))); > > > > > > >=20 > > > > > > > and for UDP, this patch is dereferencing buffer pointers only= , not > > > > > > > pointers to headers. =20 > > > > > >=20 > > > > > > Ok... but my point remains, I'm not seeing that passing the add= ress by > > > > > > value actually helps - it just seems to change whether we need = to > > > > > > handle the unaligned load in the caller or the callee. =20 > > > > >=20 > > > > > For UDP and IPv4 (from 6/9): > > > > >=20 > > > > > + b->iph.check =3D csum_ip4_header(b->iph.tot_len, IPPROTO_= UDP, > > > > > + b->iph.saddr, b->iph.daddr= ); > > > > >=20 > > > > > and for IPv6 (this patch): > > > > >=20 > > > > > + b->uh.check =3D csum(&b->uh, ntohs(b->ip6h.payload_len), > > > > > + proto_ipv6_header_psum(b->ip6h.payload= _len, > > > > > + IPPROTO_UDP, > > > > > + b->ip6h.saddr, > > > > > + b->ip6h.daddr))= ; > > > > >=20 > > > > > these cause loads starting from 'b', which is aligned, instead of > > > > > passing 'iph' or 'ip6h', unaligned, and loading from there. = =20 > > > >=20 > > > > No... the loads are still from b->ip6h.saddr, b->ip6h.daddr and > > > > b->ip6h.payload_len. =20 > > >=20 > > > It depends how we define "loading from" -- the problem, in general, i= s > > > not the memory location per se, the problem is dereferencing memory > > > pointers. > > >=20 > > > I plan to try an example on MIPS in a bit [...] =20 > >=20 > > Actually, armhf first (for clarity): > >=20 > > $ cat align.c > > #include > > #include > >=20 > > struct disarray { > > uint8_t oops; > > uint32_t v1; > > uint32_t v2; > > } __attribute__((packed, aligned(__alignof__(unsigned int)))); > >=20 > > void f1(uint32_t *v1) { > > *v1 +=3D 42; > > } > >=20 > > uint32_t f2(uint32_t v2) { > > return v2++; > > } > >=20 > > int main() > > { > > struct disarray d =3D { 0x55, 0xaa, 0xaa }; > >=20 > > f1(&d.v1); > > f2(d.v2); > >=20 > > fprintf(stdout, "%08x %08x", d.v1, d.v2); > > } > >=20 > > $ arm-linux-gnueabihf-gcc-12 -g -O0 -fno-stack-protector -fomit-frame-p= ointer -mno-unaligned-access -o align align.c > > align.c: In function =E2=80=98main=E2=80=99: > > align.c:22:8: warning: taking address of packed member of =E2=80=98stru= ct disarray=E2=80=99 may result in an unaligned pointer value [-Waddress-of= -packed-member] > > 22 | f1(&d.v1); > > | ^~~~~ > >=20 > > $ arm-linux-gnueabihf-objdump -S --disassemble=3Dmain align > > [...] > > f1(&d.v1); > > 562:=09ab01 =09add=09r3, sp, #4 > > 564:=093301 =09adds=09r3, #1 > > 566:=094618 =09mov=09r0, r3 > > 568:=09f7ff ffde =09bl=09528 > > [...] > >=20 > > before the call to f1(), the address in r3 is not aligned (we just > > added #1), despite -mno-unaligned-access. I guess gcc can only warn > > about that, but not fix it. > >=20 > > This: > > https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html > >=20 > > says: > > -munaligned-access > > -mno-unaligned-access > >=20 > > Enables (or disables) reading and writing of 16- and 32- bit values= from addresses that are not 16- or 32- bit aligned. By default unaligned a= ccess is disabled for all pre-ARMv6, all ARMv6-M and for ARMv8-M Baseline a= rchitectures, and enabled for all other architectures. If unaligned access = is not enabled then words in packed data structures are accessed a byte at = a time.=20 > >=20 > > Implying, I guess, that on those architectures unaligned accesses > > shouldn't be done. I think Thumb mode also has issues with this, by > > the way.=20 > >=20 > > And in f1() we just have a ldr from that address (passed on r0): > > void f1(uint32_t *v1) { > > 528:=09b082 =09sub=09sp, #8 > > 52a:=099001 =09str=09r0, [sp, #4] > > *v1 +=3D 42; > > 52c:=099b01 =09ldr=09r3, [sp, #4] > > 52e:=09681b =09ldr=09r3, [r3, #0] > > 530:=09f103 022a =09add.w=09r2, r3, #42=09@ 0x2a > >=20 > > $ arm-linux-gnueabihf-objdump -S --disassemble=3Df1 align > > [...] > > *v1 +=3D 42; > > 52c:=099b01 =09ldr=09r3, [sp, #4] > > 52e:=09681b =09ldr=09r3, [r3, #0] > > 530:=09f103 022a =09add.w=09r2, r3, #42=09@ 0x2a > >=20 > > ...but the call to f2() is fine: we load with offset 8 from the stack > > pointer, shift word right, load from offset 12, shift word left, OR: > >=20 > > $ arm-linux-gnueabihf-objdump -S --disassemble=3Dmain align > > [...] > > f2(d.v2); > > 56c:=099b02 =09ldr=09r3, [sp, #8] > > 56e:=090a1b =09lsrs=09r3, r3, #8 > > 570:=09f89d 200c =09ldrb.w=09r2, [sp, #12] > > 574:=090612 =09lsls=09r2, r2, #24 > > 576:=094313 =09orrs=09r3, r2 > > 578:=094618 =09mov=09r0, r3 > > 57a:=09f7ff ffe0 =09bl=0953e > > [...] =20 >=20 > Huh. Ok, so I guess the compiler realises it's doing a load from a > packed structure and generates the necessary fixup code. I thought it > would only consider the type of the actually loaded value. Well, it can't just do that, because otherwise we couldn't use packed structures on any architecture that doesn't support unaligned accesses, right? Once you pass a pointer to an unaligned value, though, the fixup information is lost, and the compiler models a function as simply taking a given pointer with a given type: the model doesn't include information as to where the value is stored. > Are you sure it still does this correctly when optimization is enabled? I'm fairly sure, even though I couldn't find a "normative" reference (at least as far as GNU extensions are concerned) guaranteeing that (but I didn't try hard). I had to move f1() and f2() to different compilation units, otherwise with -O2 they get merged into main(): $ cat align.c #include #include struct disarray { uint8_t oops; uint32_t v1; uint32_t v2; } __attribute__((packed, aligned(__alignof__(unsigned int)))); void f1(uint32_t *v1); uint32_t f2(uint32_t v2); int main() { struct disarray d =3D { 0x55, 0xaa, 0xaa }; f1(&d.v1); f2(d.v2); fprintf(stdout, "%08x %08x", d.v1, d.v2); } $ cat align_f1.c #include void f1(uint32_t *v1) { *v1 +=3D 42; } $ cat align_f2.c #include uint32_t f2(uint32_t v2) { return v2++; } $ arm-linux-gnueabihf-gcc-12 -g -O2 -fno-stack-protector -fomit-frame-point= er -mno-unaligned-access -o align align.c align_f1.c align_f2.c align.c: In function =E2=80=98main=E2=80=99: align.c:18:8: warning: taking address of packed member of =E2=80=98struct d= isarray=E2=80=99 may result in an unaligned pointer value [-Waddress-of-pac= ked-member] 18 | f1(&d.v1); | ^~~~~ ...note the usual dance before f2() is called, but not before the call to f1(): $ arm-linux-gnueabihf-objdump -S --disassemble=3Dmain align [...] struct disarray d =3D { 0x55, 0xaa, 0xaa }; 43e:=09e903 0007 =09stmdb=09r3, {r0, r1, r2} f1(&d.v1); 442:=09f10d 0005 =09add.w=09r0, sp, #5 446:=09f000 f8a5 =09bl=09594 f2(d.v2); 44a:=09f89d 300c =09ldrb.w=09r3, [sp, #12] 44e:=099802 =09ldr=09r0, [sp, #8] 450:=09061b =09lsls=09r3, r3, #24 452:=09ea43 2010 =09orr.w=09r0, r3, r0, lsr #8 456:=09f000 f8a1 =09bl=0959c [...] --=20 Stefano