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 781975A0271 for ; Mon, 4 Mar 2024 23:47:59 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709592478; 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=fIFmoY2xB9JR0KAn0aX+JH7EdrxcruGkHT9Nb+85zBo=; b=AYkMLhctjhlEyfWko30H40g4CUtZMCYdMDtZH3derMDGASqUEI5ghIRKaAO2oxL4pHV8rE eHnDmh3j3YE4qTocWlC/pX/RelTecKoWzp2ZwxRX5qhBDBhulg7gj0jGd48LnHoO8GWERv b03A597z4EQHhi8fJxYgWRx3vxdbUuE= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-298-e0SKhzC-P7qiC_VcSoV5eg-1; Mon, 04 Mar 2024 17:47:56 -0500 X-MC-Unique: e0SKhzC-P7qiC_VcSoV5eg-1 Received: by mail-ed1-f71.google.com with SMTP id 4fb4d7f45d1cf-56451e5801dso8405a12.3 for ; Mon, 04 Mar 2024 14:47:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709592475; x=1710197275; 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=mbRjfmxgB0wNgaNVamQe5fuq8YrVBTBf+Yz48qF3IHQ=; b=NydjHSY2/s2qmrFM20ON1U09i/yage03S/wMU5cMI+jjyr/dgFJxPKHuGvVm2YqxT3 rj1pvUbUw7yGX/oDuxfx3jC/39cVRhWk31wQPrzo74Ggj/P1G1QnnSq/ut2XyghuRnVt TL8JBV64u9csBBFmKcz/z040TfKH36FGSLvBdxxirjXoZvzlg+KXiVbtt7Vlk3nuRdGv FB38er4y+RI4XgqGVaPm/MRx67i/yRHRioakQn7N9yX8bYgQxuLbZYbBU0extilYktt2 bGIcomUV49ec6VrxchUyvJcEhZWOzFXIP379cYpVbwMLI53vhe8kfELX16kUa3+bBZGN anIw== X-Forwarded-Encrypted: i=1; AJvYcCUV/Pb3xh2Y4V1GM/oUxhyPjWghcPYQsSxPyKJRT3RIpVPBPI1tzJw6zlG45a5rFKQLj8zqU75pbwpiIs6OSKuCsc/1 X-Gm-Message-State: AOJu0YyIFRTo21TFQMl+TauRVxq18A1qkRA6w7bsYlkSO5C3/ObPYF8W BL+CdgJtSBo/UQrBNWzL+5gAmMxQYoMQflAgfi8WEbUm3fnQDteV2URNF6KynHcabUw5irEbSHU f1GuFsKIhTP5nfxdY6CHMxRc+uXHQ74hJxtSitiXWlkn0T7A47A== X-Received: by 2002:a50:cbc4:0:b0:567:504e:e779 with SMTP id l4-20020a50cbc4000000b00567504ee779mr2490522edi.25.1709592475413; Mon, 04 Mar 2024 14:47:55 -0800 (PST) X-Google-Smtp-Source: AGHT+IHgFmLQtKPBX8qeX4wjpzdx4DU0vLtrWdsBndTA+72ZuUV9ZbOIRJgitHWoBPj8pC9mP8Pw4g== X-Received: by 2002:a50:cbc4:0:b0:567:504e:e779 with SMTP id l4-20020a50cbc4000000b00567504ee779mr2490513edi.25.1709592474975; Mon, 04 Mar 2024 14:47:54 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id q28-20020a056402033c00b005659ea1caf0sm5080649edw.6.2024.03.04.14.47.51 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 04 Mar 2024 14:47:52 -0800 (PST) Date: Mon, 4 Mar 2024 23:47:17 +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: <20240304234717.5f697efd@elisabeth> In-Reply-To: <20240304120040.1cebc230@elisabeth> References: <20240217150725.661467-8-lvivier@redhat.com> <04c99072-02ea-46a9-aac6-23116cb05fa1@redhat.com> <20240229080509.4f534831@elisabeth> <20240229095625.557367ab@elisabeth> <20240229151553.60d5cf18@elisabeth> <20240301075651.42ec7145@elisabeth> <20240304120040.1cebc230@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: JQDOLWHIABAUCHKOV25LEHZ3LNSEWA56 X-Message-ID-Hash: JQDOLWHIABAUCHKOV25LEHZ3LNSEWA56 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 Mon, 4 Mar 2024 12:00:40 +0100 Stefano Brivio wrote: > 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 wrot= e: =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 Vivier = wrote: =20 > > > > > > > > > > On 2/19/24 04:08, David Gibson wrote: =20 > > > > > > > > > > > On Sat, Feb 17, 2024 at 04:07:23PM +0100, Laurent Viv= ier wrote: =20 > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > =20 > > > > > > > > > > > > +/** > > > > > > > > > > > > + * proto_ipv6_header_psum() - Calculates the parti= al 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 header > > > > > > > > > > > > + */ > > > > > > > > > > > > +uint32_t proto_ipv6_header_psum(uint16_t payload_l= en, 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 valu= e, which might > > > > > > > > > > > not be what we want. =20 > > > > > > > > > >=20 > > > > > > > > > > The idea here is to avoid the pointer alignment problem= (&ip6h->saddr and > > > > > > > > > > &ip6h->daddr can be misaligned). =20 > > > > > > > > >=20 > > > > > > > > > Ah, right. That's a neat idea, but I'm not sure it reall= y helps: I > > > > > > > > > think it will just move the misaligned access from inside= the function > > > > > > > > > to the call site, where we try to marshal the parameter f= rom something > > > > > > > > > unaligned. =20 > > > > > > > >=20 > > > > > > > > I haven't tested this yet, but note that this is generally = okay: the > > > > > > > > problem is *dereferencing* an unaligned pointer. But if you= load memory > > > > > > > > from an aligned pointer, and extract a value from this memo= ry, it's all > > > > > > > > fine. =20 > > > > > > >=20 > > > > > > > Right, that's kind of what I'm getting at. Assuming this val= ue starts > > > > > > > in an unaligned buffer, then in order to pass this by value t= he caller > > > > > > > will need to load from that unaligned pointer. AFAIK, the co= mpiler > > > > > > > will base the type of loads only on the pointed to type, whic= h isn't > > > > > > > changed whether we dereference in the caller or the callee. > > > > > > > =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 rather = than the > > > > > > > former. =20 > > > > > >=20 > > > > > > Oh, oops, I didn't realise this was the case (I haven't reviewe= d 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, no= t > > > > > pointers to headers. =20 > > > >=20 > > > > Ok... but my point remains, I'm not seeing that passing the address= 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, is > not the memory location per se, the problem is dereferencing memory > pointers. >=20 > I plan to try an example on MIPS in a bit [...] Actually, armhf first (for clarity): $ 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) { *v1 +=3D 42; } uint32_t f2(uint32_t v2) { return v2++; } int main() { struct disarray d =3D { 0x55, 0xaa, 0xaa }; f1(&d.v1); f2(d.v2); fprintf(stdout, "%08x %08x", d.v1, d.v2); } $ arm-linux-gnueabihf-gcc-12 -g -O0 -fno-stack-protector -fomit-frame-point= er -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=98struct d= isarray=E2=80=99 may result in an unaligned pointer value [-Waddress-of-pac= ked-member] 22 | f1(&d.v1); | ^~~~~ $ 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 [...] 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. This: https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html says: -munaligned-access -mno-unaligned-access Enables (or disables) reading and writing of 16- and 32- bit values fro= m addresses that are not 16- or 32- bit aligned. By default unaligned acces= s is disabled for all pre-ARMv6, all ARMv6-M and for ARMv8-M Baseline archi= tectures, and enabled for all other architectures. If unaligned access is n= ot enabled then words in packed data structures are accessed a byte at a ti= me.=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 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 $ 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 ...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: $ 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 [...] Now on to MIPS (MIPS32): $ mips-linux-gnu-gcc-12 -g -O0 -fno-stack-protector -fomit-frame-pointer -m= no-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=98struct d= isarray=E2=80=99 may result in an unaligned pointer value [-Waddress-of-pac= ked-member] 22 | f1(&d.v1); | ^~~~~ $ mips-linux-gnu-objdump -S --disassemble=3Dmain align [...] f1(&d.v1); 7bc:=0927a20019 =09addiu=09v0,sp,25 7c0:=0900402025 =09move=09a0,v0 7c4:=098f82802c =09lw=09v0,-32724(gp) 7c8:=090040c825 =09move=09t9,v0 7cc:=090411ffe0 =09bal=09750 7d0:=0900000000 =09nop 7d4:=098fbc0010 =09lw=09gp,16(sp) [...] '&d.v1' is passed in a0, again unaligned (stack pointer plus 25). And f1() uses it just like that: $ mips-linux-gnu-objdump -S --disassemble=3Df1 align [...] void f1(uint32_t *v1) { 750:=09afa40000 =09sw=09a0,0(sp) *v1 +=3D 42; 754:=098fa20000 =09lw=09v0,0(sp) 758:=098c420000 =09lw=09v0,0(v0) 75c:=092443002a =09addiu=09v1,v0,42 [...] while the call to f2() is, again, fine: $ mips-linux-gnu-objdump -S --disassemble=3Dmain align f2(d.v2); 7e0:=098ba2001d =09lwl=09v0,29(sp) 7e4:=099ba20020 =09lwr=09v0,32(sp) 7e8:=0900402025 =09move=09a0,v0 7ec:=098f828030 =09lw=09v0,-32720(gp) 7f0:=090040c825 =09move=09t9,v0 7f4:=090411ffdf =09bal=09774 7f8:=0900000000 =09nop 7fc:=098fbc0010 =09lw=09gp,16(sp) two loads, from stack pointer + 29 and stack pointer + 32. MIPS32 has lwl and lwr (the infamous US4814976A patent, now expired) to avoid load plus shift plus OR. Now, you might argue that what I'm describing here might simply be gcc's behaviour, and if gcc avoids unaligned loads as long as we don't pass unaligned pointers around, that's not any better for us -- other compilers might do things differently. And... yes, packed structures are actually a GNU extension: C standards don't say anything about loads like my f1(d.v2) call above, so all I'm showing here is that a particular compiler is fine with these accesses, but not unaligned pointers. On the other hand, this seems to be a well established behaviour, and I don't think we could realistically drop every load of unaligned *values*. Unaligned pointers, we currently don't dereference any, because gcc warns otherwise. So, practically speaking, I guess as long as we avoid dereferencing unaligned pointers, we should be fine? --=20 Stefano