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

[Andrew Lunn]

On Mon, Sep 10, 2018 at 07:08:38PM -0600, Jason A. Donenfeld wrote:
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> Cc: David Miller <davem@xxxxxxxxxxxxx>
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---

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@xxxxxxxxx>. 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