Re: [PATCH v3] net: ipv4: move tcp_fastopen server side code to SipHash library

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Jun 17, 2019 at 10:09:33AM +0200, Ard Biesheuvel wrote:
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index c23019a3b264..9ea0e71f5c6a 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -58,12 +58,7 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
>  
>  /* TCP Fast Open Cookie as stored in memory */
>  struct tcp_fastopen_cookie {
> -	union {
> -		u8	val[TCP_FASTOPEN_COOKIE_MAX];
> -#if IS_ENABLED(CONFIG_IPV6)
> -		struct in6_addr addr;
> -#endif
> -	};
> +	u64	val[TCP_FASTOPEN_COOKIE_MAX / sizeof(u64)];
>  	s8	len;
>  	bool	exp;	/* In RFC6994 experimental option format */
>  };

Is it okay that the cookies will depend on CPU endianness?

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 96e0e53ff440..184930b02779 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1628,9 +1628,9 @@ bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
>  
>  /* Fastopen key context */
>  struct tcp_fastopen_context {
> -	struct crypto_cipher	*tfm[TCP_FASTOPEN_KEY_MAX];
> -	__u8			key[TCP_FASTOPEN_KEY_BUF_LENGTH];
> -	struct rcu_head		rcu;
> +	__u8		key[TCP_FASTOPEN_KEY_MAX][TCP_FASTOPEN_KEY_LENGTH];
> +	int		num;
> +	struct rcu_head	rcu;
>  };

Why not use 'siphash_key_t' here?  Then the (potentially alignment-violating)
cast in __tcp_fastopen_cookie_gen_cipher() wouldn't be needed.

>  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
>  			      void *primary_key, void *backup_key,
>  			      unsigned int len)
> @@ -115,11 +75,20 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
>  	struct fastopen_queue *q;
>  	int err = 0;
>  
> -	ctx = tcp_fastopen_alloc_ctx(primary_key, backup_key, len);
> -	if (IS_ERR(ctx)) {
> -		err = PTR_ERR(ctx);
> +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx) {
> +		err = -ENOMEM;
>  		goto out;
>  	}
> +
> +	memcpy(ctx->key[0], primary_key, len);
> +	if (backup_key) {
> +		memcpy(ctx->key[1], backup_key, len);
> +		ctx->num = 2;
> +	} else {
> +		ctx->num = 1;
> +	}
> +
>  	spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
>  	if (sk) {
>  		q = &inet_csk(sk)->icsk_accept_queue.fastopenq;

Shouldn't there be a check that 'len == TCP_FASTOPEN_KEY_LENGTH'?  I see that
all callers pass that, but it seems unnecessarily fragile for this to accept
short lengths and leave uninitialized memory in that case.

- Eric