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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9815 bytes --]

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 <david@gibson.dropbear.id.au>
> ---
>  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.

>  };
> +static_assert(sizeof(union inany_addr) == sizeof(struct in6_addr));
>  
>  /** 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_addr *aa, in_port_t *port,
>  	}
>  }
>  
> +/** 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 <sbrivio@redhat.com>
> - */
> -
> -#include <stddef.h>
> -#include <stdint.h>
> -
> -#include "siphash.h"
> -
> -/**
> - * siphash_8b() - Table index or timestamp offset for TCP over IPv4 (8 bytes 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 = 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 in)
> - * @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 = SIPHASH_INIT(k);
> -	uint32_t *in32 = (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 = SIPHASH_INIT(k);
> -	uint32_t *in32 = (uint32_t *)in;
> -	int i;
> -
> -	for (i = 0; i < 2; i++, in32 += 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 = SIPHASH_INIT(k);
> -	uint64_t *in64 = (uint64_t *)in;
> -	int i;
> -
> -	for (i = 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 in)
> - * @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 = SIPHASH_INIT(k);
> -	uint32_t *in32 = (uint32_t *)in;
> -	int i;
> -
> -	for (i = 0; i < 4; i++, in32 += 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_conn *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 = {
> -		*faddr, eport, fport
> -	};
> -	uint64_t b = 0;
> +	struct siphash_state state = SIPHASH_INIT(c->tcp.hash_secret);
> +	uint64_t hash;
>  
> -	b = siphash_20b((uint8_t *)&in, c->tcp.hash_secret);
> +	inany_siphash_feed(&state, faddr);
> +	hash = siphash_final(&state, 20, (uint64_t)eport << 16 | fport);
>  
> -	return (unsigned int)(b % TCP_HASH_TABLE_SIZE);
> +	return (unsigned int)(hash % TCP_HASH_TABLE_SIZE);
>  }
>  
>  /**
> @@ -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 = 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 = {
> -		.src = conn->faddr,
> -		.srcport = conn->fport,
> -		.dstport = conn->eport,
> -	};
>  	uint64_t hash;
>  	uint32_t ns;
>  
> @@ -1833,9 +1819,11 @@ static void tcp_seq_init(const struct ctx *c, struct 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 = aany;
>  
> -	hash = siphash_36b((uint8_t *)&in, c->tcp.hash_secret);
> +	inany_siphash_feed(&state, &conn->faddr);
> +	inany_siphash_feed(&state, &aany);
> +	hash = siphash_final(&state, 36,
> +			     (uint64_t)conn->fport << 16 | conn->eport);
>  
>  	/* 32ns ticks, overflows 32 bits every 137s */
>  	ns = (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"
>  
>  #include "tcp_conn.h"

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]