From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id C9A3E5A0272 for ; Thu, 28 Dec 2023 06:58:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1703743124; bh=k5ZEp2JbwvV8q+f8tdVkFVxejkAr5CZyx0KaNWX5Kfs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UZibhHKxEVFJ8tH8R1Z+845tHICCA5xPVZcMBN7Yb3VfTVIGktzoOVaNkKchsJinx Uc1r1gXRdz0Rlm7TATG/gf6li0uyu77I2mDN/x/wH30fna2Rd7gronhoutR+VF5E5g MV32O2xXSeDhc1L5n7LHCa3K0NMClq0DAk56IYXkkNSnw4H0BNTmPBIr1nl+b8TmAy 7w+M7Fcj2ejQ88vLbeoFRBkKJ0A+B9brh9Lt004ObBjqhTctUEmIxImRmXBnJ683gt xOJK/871nxxZrP1UHZow9FD7m5FENtNg2CJTFmwIFhcMw3Hz+Te6xj+UdMfttugEzq 69nsTFCwzE1SA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4T0yVr1qxFz4wnv; Thu, 28 Dec 2023 16:58:44 +1100 (AEDT) Date: Thu, 28 Dec 2023 13:42:25 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af() Message-ID: References: <20231207143140.1851378-1-david@gibson.dropbear.id.au> <20231207143140.1851378-2-david@gibson.dropbear.id.au> <20231227212506.75eb41c7@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yRQMBBAeFrlqnVoc" Content-Disposition: inline In-Reply-To: <20231227212506.75eb41c7@elisabeth> Message-ID-Hash: V2ILCFTK4KX3Q2UATT5DYHM44K35NMO6 X-Message-ID-Hash: V2ILCFTK4KX3Q2UATT5DYHM44K35NMO6 X-MailFrom: dgibson@gandalf.ozlabs.org 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: --yRQMBBAeFrlqnVoc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote: > On Fri, 8 Dec 2023 01:31:33 +1100 > David Gibson wrote: >=20 > > This takes a struct in_addr * (i.e. an IPv4 address), although it's > > explicitly supposed to handle IPv6 as well. Both its caller and sock_l= 4() > > which it calls use a void * for the address, which can be either an in_= addr > > or an in6_addr. > >=20 > > We get away with this, because we don't do anything with the pointer ot= her > > than transfer it from the caller to sock_l4(), but it's misleading. And > > quite possibly technically UB, because C is like that. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index f506cfd..bda95b2 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_= ref ref, uint32_t events) > > * Return: fd for the new listening socket, negative error code on fai= lure > > */ > > static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t por= t, > > - const struct in_addr *addr, const char *ifname) > > + const void *addr, const char *ifname) >=20 > This is obviously correct. >=20 > However, after a lot of thinking: (gcc) optimisations based on > Type-Based Alias Analysis, which we don't disable on this path, could, > now, happily defer filling 'addr' with inet_pton() in conf_ports() to a > point *after* the tcp_sock_init() call. Hrm... possibly. The fact that the addr variable in conf_ports() is a char array, not a struct in*_addr might save us. I think replacing it with a union of an in_addr and in6_addr would also be ok. > Without this patch, at least 32 bits must be updated before the call. I'm not sure that's correct. If the compiler is allowed to assume that a char[] and a void * aren't aliased (even if they clearly are), then I'd expect it to also be allowed to assume that a char[] and a struct in_addr * aren't aliased. > It might sound like a joke because... it actually is. But look at what > we had to do for the functions in checksum.c. We pass const void *buf, > and anything that buf points to can be updated (with TBAA) after the > call. >=20 > I don't see any conceptual difference between this case and those > functions. >=20 > Anyway, that won't reasonably happen here, and in any case this would > have been broken for IPv6, so I'll go ahead and apply this. >=20 > But, eventually, I think we should switch all these usages to union > inany_addr *. So, we may be able to use union inany_addr in some places, but that's not the same thing as this: inany_addr carries IPv4 addresses as mapped IPv6 addresses, it's not switched on a separate af parameter. We could, of course, define a new type as a simple union of in_addr and in6_addr. --=20 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 --yRQMBBAeFrlqnVoc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWM4IEACgkQzQJF27ox 2Gd41g//eUud4zM51c2Ek5JuyBFlE+HTG/SvU1d3Lw7ykHosvEHOP6tCKu/i46YR /FGN+zyYGB6OXhlbNY1U8Sg5AYcEqX5yXyEofzLk6RdCnE0/xpfwPVDOg0GpmdLL M5OspA1PCs7VFwWPx+Cl6FfL2xmb84KVBIcxscGrWrinoWKsE0GrUEDOt+zMH7V7 q1Suv6tUs4OdI0dztruMjL/8fRwpAT7n75SHDDE67zG15Rbry50q7m8LWySkLsBB p0JszzE8llOHsOUWZy8N4qTZd9XsWxm7V51ITZ5B/p2KK2+NxXZRcOwMDY0XvyDB EEhKIsvULbcQbAHT8hGQkh4cIydWZfKEchWU1nnlM/9Pgm+W6nmNq0sX88FCyuhS akn70/sS14DsJK0YFdv5xapkWscvhgURMF7hEmkVVNfH3TNF2k36qFfw9lsasOl8 ubyUdfSVgVDtMTdHIZboHy+2Ad/3pH5vifsXLmg72U4JCsWWJbqX/WP7QFpmpdEo uK8IE5JAzudAJpeuoGvchG/En7hkzIIxZKx2HjYuCOLDZBQi90D1OR803EvnRU5k bAWfevM/J0+nl8uKGze0UaCscbL3Ij7luivyzNyIUBF2O6VIshXFeyu6NTDwO08X 9hVikgkI495/bwn3kP36Xji3hL1WdihJ6HBc2ImPG1rfxDbJFp8= =cuzs -----END PGP SIGNATURE----- --yRQMBBAeFrlqnVoc--