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 A08275A026D for ; Tue, 26 Sep 2023 08:29:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1695709767; bh=1oQ9wjmemmibpudFVMFlZVUK9N9l76FBYsp8s28kv7Y=; h=Date:From:To:Subject:References:In-Reply-To:From; b=hHOxp0brQO/XZk5SNp+FjtLhOjhTyYHfMc4tmCl154EbzLtkL9JLNCkz3tKVjyNlb ooYIzS0BqiBsFgPFLNCYJ5oiah/LesV0GXpB8omxoIFXA+nN77sGEdwG94ile5EC6o bvrpd1M4uEAY90BvCAS/2vp2SopPkg9rIq0SP3s4= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RvqbC71Hyz4xNs; Tue, 26 Sep 2023 16:29:27 +1000 (AEST) Date: Tue, 26 Sep 2023 16:23:45 +1000 From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: Re: [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions Message-ID: References: <20230922140630.3184256-1-david@gibson.dropbear.id.au> <20230922140630.3184256-11-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WWfCZx2wl0Llr2nR" Content-Disposition: inline In-Reply-To: <20230922140630.3184256-11-david@gibson.dropbear.id.au> Message-ID-Hash: UQH4WF227VKRWZSCODSVTDPZXPRRO2BX X-Message-ID-Hash: UQH4WF227VKRWZSCODSVTDPZXPRRO2BX 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 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: --WWfCZx2wl0Llr2nR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Sep 23, 2023 at 12:06:30AM +1000, David Gibson wrote: > We have a bunch of variants of the siphash functions for different data > sizes. The callers, in tcp.c, need to pack the various values they want = to > hash into a temporary structure, then call the appropriate version. We c= an > avoid the copy into the temporary by directly using the incremental > siphash functions. >=20 > The length specific hash functions also have an undocumented constraint > that the data pointer they take must, in fact, be aligned to avoid > unaligned accesses, which may cause crashes on some architectures. >=20 > So, prefer the incremental approach and remove the length-specific > functions. >=20 > Signed-off-by: David Gibson > --- > Makefile | 2 +- > inany.h | 16 ++++++- > siphash.c | 121 --------------------------------------------------- > tcp.c | 32 +++++--------- > tcp_splice.c | 1 + > 5 files changed, 27 insertions(+), 145 deletions(-) > delete mode 100644 siphash.c >=20 > diff --git a/Makefile b/Makefile > index 4435bd6..ec3c3fb 100644 > --- a/Makefile > +++ b/Makefile > @@ -45,7 +45,7 @@ FLAGS +=3D -DDUAL_STACK_SOCKETS=3D$(DUAL_STACK_SOCKETS) > =20 > PASST_SRCS =3D arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igm= p.c \ > isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c passt.c \ > - pasta.c pcap.c siphash.c tap.c tcp.c tcp_splice.c udp.c util.c > + pasta.c pcap.c tap.c tcp.c tcp_splice.c udp.c util.c > QRAP_SRCS =3D qrap.c > SRCS =3D $(PASST_SRCS) $(QRAP_SRCS) > =20 > diff --git a/inany.h b/inany.h > index aadb20b..266d101 100644 > --- a/inany.h > +++ b/inany.h > @@ -14,8 +14,9 @@ > * @v4mapped.zero: All zero-bits for an IPv4 address > * @v4mapped.one: All one-bits for an IPv4 address > * @v4mapped.a4: If @a6 is an IPv4 mapped address, the IPv4 address > + * @u64: As an array of u64s (solely for hashing) > * > - * @v4mapped shouldn't be accessed except via helpers. > + * @v4mapped and @u64 shouldn't be accessed except via helpers. > */ > union inany_addr { > struct in6_addr a6; > @@ -24,7 +25,9 @@ union inany_addr { > uint8_t one[2]; > struct in_addr a4; > } v4mapped; > + uint64_t u64[2]; I realised this change alters the alignment of inany from 4 bytes to 8 bytes, which causes problems for things I have in the works. Revised version coming. > }; > +static_assert(sizeof(union inany_addr) =3D=3D sizeof(struct in6_addr)); > =20 > /** inany_v4 - Extract IPv4 address, if present, from IPv[46] address > * @addr: IPv4 or IPv6 address > @@ -94,4 +97,15 @@ static inline void inany_from_sockaddr(union inany_add= r *aa, in_port_t *port, > } > } > =20 > +/** inany_siphash_feed- Fold IPv[46] address into an in-progress siphash > + * @state: siphash state > + * @aa: inany to hash > + */ > +static inline void inany_siphash_feed(struct siphash_state *state, > + const union inany_addr *aa) > +{ > + siphash_feed(state, aa->u64[0]); > + siphash_feed(state, aa->u64[1]); > +} > + > #endif /* INANY_H */ > diff --git a/siphash.c b/siphash.c > deleted file mode 100644 > index d2b068c..0000000 > --- a/siphash.c > +++ /dev/null > @@ -1,121 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-or-later > - > -/* PASST - Plug A Simple Socket Transport > - * for qemu/UNIX domain socket mode > - * > - * PASTA - Pack A Subtle Tap Abstraction > - * for network namespace/tap device mode > - * > - * siphash.c - SipHash routines > - * > - * Copyright (c) 2020-2021 Red Hat GmbH > - * Author: Stefano Brivio > - */ > - > -#include > -#include > - > -#include "siphash.h" > - > -/** > - * siphash_8b() - Table index or timestamp offset for TCP over IPv4 (8 b= ytes in) > - * @in: Input data (remote address and two ports, or two addresses) > - * @k: Hash function key, 128 bits > - * > - * Return: the 64-bit hash output > - */ > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > -/* cppcheck-suppress unusedFunction */ > -uint64_t siphash_8b(const uint8_t *in, const uint64_t *k) > -{ > - struct siphash_state state =3D SIPHASH_INIT(k); > - > - siphash_feed(&state, *(uint64_t *)in); > - > - return siphash_final(&state, 8, 0); > -} > - > -/** > - * siphash_12b() - Initial sequence number for TCP over IPv4 (12 bytes i= n) > - * @in: Input data (two addresses, two ports) > - * @k: Hash function key, 128 bits > - * > - * Return: the 64-bit hash output > - */ > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > -/* cppcheck-suppress unusedFunction */ > -uint64_t siphash_12b(const uint8_t *in, const uint64_t *k) > -{ > - struct siphash_state state =3D SIPHASH_INIT(k); > - uint32_t *in32 =3D (uint32_t *)in; > - > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > - > - return siphash_final(&state, 12, *(in32 + 2)); > -} > - > -/** > - * siphash_20b() - Table index for TCP over IPv6 (20 bytes in) > - * @in: Input data (remote address, two ports) > - * @k: Hash function key, 128 bits > - * > - * Return: the 64-bit hash output > - */ > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > -uint64_t siphash_20b(const uint8_t *in, const uint64_t *k) > -{ > - struct siphash_state state =3D SIPHASH_INIT(k); > - uint32_t *in32 =3D (uint32_t *)in; > - int i; > - > - for (i =3D 0; i < 2; i++, in32 +=3D 2) > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > - > - return siphash_final(&state, 20, *in32); > -} > - > -/** > - * siphash_32b() - Timestamp offset for TCP over IPv6 (32 bytes in) > - * @in: Input data (two addresses) > - * @k: Hash function key, 128 bits > - * > - * Return: the 64-bit hash output > - */ > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > -/* cppcheck-suppress unusedFunction */ > -uint64_t siphash_32b(const uint8_t *in, const uint64_t *k) > -{ > - struct siphash_state state =3D SIPHASH_INIT(k); > - uint64_t *in64 =3D (uint64_t *)in; > - int i; > - > - for (i =3D 0; i < 4; i++, in64++) > - siphash_feed(&state, *in64); > - > - return siphash_final(&state, 32, 0); > -} > - > -/** > - * siphash_36b() - Initial sequence number for TCP over IPv6 (36 bytes i= n) > - * @in: Input data (two addresses, two ports) > - * @k: Hash function key, 128 bits > - * > - * Return: the 64-bit hash output > - */ > -/* NOLINTNEXTLINE(clang-diagnostic-unknown-attributes) */ > -__attribute__((optimize("-fno-strict-aliasing"))) /* See csum_16b() */ > -uint64_t siphash_36b(const uint8_t *in, const uint64_t *k) > -{ > - struct siphash_state state =3D SIPHASH_INIT(k); > - uint32_t *in32 =3D (uint32_t *)in; > - int i; > - > - for (i =3D 0; i < 4; i++, in32 +=3D 2) > - siphash_feed(&state, (uint64_t)(*(in32 + 1)) << 32 | *in32); > - > - return siphash_final(&state, 36, *in32); > -} > diff --git a/tcp.c b/tcp.c > index 9f28020..18ceed1 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -1165,18 +1165,13 @@ static int tcp_hash_match(const struct tcp_tap_co= nn *conn, > static unsigned int tcp_hash(const struct ctx *c, const union inany_addr= *faddr, > in_port_t eport, in_port_t fport) > { > - struct { > - union inany_addr faddr; > - in_port_t eport; > - in_port_t fport; > - } __attribute__((__packed__)) in =3D { > - *faddr, eport, fport > - }; > - uint64_t b =3D 0; > + struct siphash_state state =3D SIPHASH_INIT(c->tcp.hash_secret); > + uint64_t hash; > =20 > - b =3D siphash_20b((uint8_t *)&in, c->tcp.hash_secret); > + inany_siphash_feed(&state, faddr); > + hash =3D siphash_final(&state, 20, (uint64_t)eport << 16 | fport); > =20 > - return (unsigned int)(b % TCP_HASH_TABLE_SIZE); > + return (unsigned int)(hash % TCP_HASH_TABLE_SIZE); > } > =20 > /** > @@ -1815,17 +1810,8 @@ static void tcp_clamp_window(const struct ctx *c, = struct tcp_tap_conn *conn, > static void tcp_seq_init(const struct ctx *c, struct tcp_tap_conn *conn, > const struct timespec *now) > { > + struct siphash_state state =3D SIPHASH_INIT(c->tcp.hash_secret); > union inany_addr aany; > - struct { > - union inany_addr src; > - in_port_t srcport; > - union inany_addr dst; > - in_port_t dstport; > - } __attribute__((__packed__)) in =3D { > - .src =3D conn->faddr, > - .srcport =3D conn->fport, > - .dstport =3D conn->eport, > - }; > uint64_t hash; > uint32_t ns; > =20 > @@ -1833,9 +1819,11 @@ static void tcp_seq_init(const struct ctx *c, stru= ct tcp_tap_conn *conn, > inany_from_af(&aany, AF_INET, &c->ip4.addr); > else > inany_from_af(&aany, AF_INET6, &c->ip6.addr); > - in.dst =3D aany; > =20 > - hash =3D siphash_36b((uint8_t *)&in, c->tcp.hash_secret); > + inany_siphash_feed(&state, &conn->faddr); > + inany_siphash_feed(&state, &aany); > + hash =3D siphash_final(&state, 36, > + (uint64_t)conn->fport << 16 | conn->eport); > =20 > /* 32ns ticks, overflows 32 bits every 137s */ > ns =3D (now->tv_sec * 1000000000 + now->tv_nsec) >> 5; > diff --git a/tcp_splice.c b/tcp_splice.c > index 5b36975..3b98260 100644 > --- a/tcp_splice.c > +++ b/tcp_splice.c > @@ -52,6 +52,7 @@ > #include "passt.h" > #include "log.h" > #include "tcp_splice.h" > +#include "siphash.h" > #include "inany.h" > =20 > #include "tcp_conn.h" --=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 --WWfCZx2wl0Llr2nR Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmUSeNsACgkQzQJF27ox 2GfIeQ//QkVnvOTzxFE0OPIlREfYRUBfS57C+Ok651AsLTzCGQZf6Fsu8McFiGGo vqWqQ8Vp+95ZlUWROamOO95hCrbDZumIUERNWZUaeqQLXl/AWWuDIcKcflnz5Jgz 6XHh7O8gZQjdJmn7Kkc57c6ruvL6H+65WgG10qVgImUfalEBa3Wv3QhYw+zOQSR+ NV0W/mFB9dz9w5kE6ch78A8WC8SkMccWRaOoiIpfB+L7dfUtjf6b/TYIdI0KxEjw rIshbtOpgh1gemq5m/qwwZeoN+5N8y1R/HrnMB/S/gtt+Zj4Z1zEeJgLGRY3j9qY 9oPgfOaSxKf/SCJP8P6w5TAlMCMp/l0FYzKBTZASJD+DDSlJ6lhSIOFGZRo6HfdE hRF6xsgEUEmQUzMMn5xeaxhDFXCD9M9zAPkaOrffrJrssWdJ2aj23VXgQP8b1aCc p4H/HbcA8lm9i+SZgKbIIc7TYd5jihkPqTUMoivqC30FfnwUuavEhA1mft3sXrau ZNLIgIkJwmnGnQvut2VkXQfr1JcQzn+nlf6DJf0VCmIAHLFkNqTSE2jRLhWq3xYZ TW8uVxjoE/VT3RmILNM9B7ILRA6FsGrC3gxAuTVIVIodaZmc4dLBnJii/ee553GE 47hCKfupsUl2bkBd/C7IPWre0H0eDY8DfCuAIxkPJ+zRBgBjz8E= =iZ8k -----END PGP SIGNATURE----- --WWfCZx2wl0Llr2nR--