From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>, passt-dev@passt.top
Subject: Re: [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions
Date: Tue, 26 Sep 2023 17:02:19 +1000 [thread overview]
Message-ID: <ZRKB++8W1tE2unFa@zatzit> (raw)
In-Reply-To: <ZRJ48UtuSKUnxbgB@zatzit>
[-- Attachment #1: Type: text/plain, Size: 10551 bytes --]
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 <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.
Actually, I might as well wait for any comments you have on v1, before
folding that into v2.
>
> > };
> > +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 --]
next prev parent reply other threads:[~2023-09-26 7:06 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-22 14:06 [PATCH 00/10] siphash: cleanups and fixes David Gibson
2023-09-22 14:06 ` [PATCH 01/10] siphash: Make siphash functions consistently return 64-bit results David Gibson
2023-09-22 14:06 ` [PATCH 02/10] siphash: Make sip round calculations an inline function rather than macro David Gibson
2023-09-22 14:06 ` [PATCH 03/10] siphash: Add siphash_feed() helper David Gibson
2023-09-22 14:06 ` [PATCH 04/10] siphash: Clean up hash finalisation with posthash_final() function David Gibson
2023-09-22 14:06 ` [PATCH 05/10] siphash: Fix bug in state initialisation David Gibson
2023-09-22 14:06 ` [PATCH 06/10] siphash: Use more hygienic state initialiser David Gibson
2023-09-27 17:04 ` Stefano Brivio
2023-09-28 1:20 ` David Gibson
2023-09-29 15:19 ` Stefano Brivio
2023-09-22 14:06 ` [PATCH 07/10] siphash: Use specific structure for internal state David Gibson
2023-09-22 14:06 ` [PATCH 08/10] siphash: Make internal helpers public David Gibson
2023-09-22 14:06 ` [PATCH 09/10] siphash, checksum: Move TBAA explanation to checksum.c David Gibson
2023-09-22 14:06 ` [PATCH 10/10] siphash: Use incremental rather than all-at-once siphash functions David Gibson
2023-09-26 6:23 ` David Gibson
2023-09-26 7:02 ` David Gibson [this message]
2023-09-27 17:05 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZRKB++8W1tE2unFa@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).