netdev - Re: [PATCH net-next v3 17/17] net: WireGuard secure network tunnel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180911133017.GJ30395@lunn.ch>
Date:   Tue, 11 Sep 2018 15:30:17 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        davem@...emloft.net, gregkh@...uxfoundation.org
Subject: Re: [PATCH net-next v3 17/17] net: WireGuard secure network tunnel

On Mon, Sep 10, 2018 at 07:08:38PM -0600, Jason A. Donenfeld wrote:
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> Cc: David Miller <davem@...emloft.net>
> Cc: Greg KH <gregkh@...uxfoundation.org>
> ---

Hi Jason

I don't know if any of the crypto people are reviewing the networking
code, but i don't think there are many networking people reviewing the
crypto code.

So please can you put the change log between versions for the
networking code here, where we can easily see it.

> +++ b/drivers/net/wireguard/allowedips.c
> @@ -0,0 +1,404 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@...c4.com>. All Rights Reserved.
> + */
> +
> +#include "allowedips.h"
> +#include "peer.h"
> +
> +struct allowedips_node {
> +	struct wireguard_peer __rcu *peer;
> +	struct rcu_head rcu;
> +	struct allowedips_node __rcu *bit[2];
> +	/* While it may seem scandalous that we waste space for v4,
> +	 * we're alloc'ing to the nearest power of 2 anyway, so this
> +	 * doesn't actually make a difference.
> +	 */
> +	u8 bits[16] __aligned(__alignof(u64));
> +	u8 cidr, bit_at_a, bit_at_b;
> +};
> +
> +static __always_inline void swap_endian(u8 *dst, const u8 *src, u8 bits)
> +{
> +	if (bits == 32)
> +		*(u32 *)dst = be32_to_cpu(*(const __be32 *)src);
> +	else if (bits == 128) {
> +		((u64 *)dst)[0] = be64_to_cpu(((const __be64 *)src)[0]);
> +		((u64 *)dst)[1] = be64_to_cpu(((const __be64 *)src)[1]);
> +	}
> +}

I see you have yet again completely ignored my request to either
1) Remove the inline
2) Argue why it should be kept.

Ignoring reviewer comments is not going to help your cause.

> +void allowedips_init(struct allowedips *table)
> +{
> +	table->root4 = table->root6 = NULL;
> +	table->seq = 1;
> +}

You have a number of global scope functions which are polluting the
namespace. Anything which is global scope needs a prefix. If my
grep-foo is correct, the prefix wg_ is currently unused. It is also
reasonably normal for static members to also make use of the prefix,
but not a must.

    Andrew