Re: [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions

On Tue, 26 Sep 2023 17:02:19 +1000 David Gibson wrote:

On Tue, Sep 26, 2023 at 04:23:45PM +1000, David Gibson wrote:

...

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 can avoid the copy into the temporary by directly using the incremental siphash functions.

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.

So, prefer the incremental approach and remove the length-specific functions.

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

diff --git a/Makefile b/Makefile index 4435bd6..ec3c3fb 100644 --- a/Makefile +++ b/Makefile @@ -45,7 +45,7 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)

PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.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 = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS)

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.

Actually, I might as well wait for any comments you have on v1, before folding that into v2.

Nothing else from my side. -- Stefano