Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 25 Aug 2018 01:00:45 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        davem@davemloft.net, Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2 17/17] net: WireGuard secure network tunnel
Message-ID: <20180824230045.GH27483@lunn.ch>
References: <20180824213849.23647-1-Jason@zx2c4.com>
 <20180824213849.23647-18-Jason@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180824213849.23647-18-Jason@zx2c4.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jason

It is normal to include after the --- what you have changed since the
previous version.

I ran checkpatch on this again.

Last time, we had:

total: 6 errors, 763 warnings, 6514 lines checked

This time, we have:

total: 8 errors, 196 warnings, 7470 lines checked

So much better with the warnings. Thanks.

The errors should be simple to fix:

ERROR: that open brace { should be on the previous line
ERROR: that open brace { should be on the previous line
ERROR: spaces required around that '=' (ctx:VxW)
ERROR: spaces required around that '=' (ctx:VxW)
ERROR: spaces required around that '=' (ctx:VxW)
ERROR: that open brace { should be on the previous line
ERROR: Use of __initconst requires a separate use of const
ERROR: trailing whitespace

One appears to be a false positive:

+static const struct {
+       bool result;
+       unsigned int msec_to_sleep_before;
+} expected_results[] __initconst = {
+       [0 ... PACKETS_BURSTABLE - 1] = { true, 0 },
+       [PACKETS_BURSTABLE] = { false, 0 },
+       [PACKETS_BURSTABLE + 1] = { true, MSEC_PER_SEC / PACKETS_PER_SECOND },
+       [PACKETS_BURSTABLE + 2] = { false, 0 },
+       [PACKETS_BURSTABLE + 3] = { true, (MSEC_PER_SEC / PACKETS_PER_SECOND) * 2 },
+       [PACKETS_BURSTABLE + 4] = { true, 0 },
+       [PACKETS_BURSTABLE + 5] = { false, 0 }
+};

Looking at some of the warnings:

WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#428: FILE: drivers/net/wireguard/allowedips.c:112:
+               BUG_ON(len >= 128); 

WARNING: Macros with flow control statements should be avoided
#5905: FILE: drivers/net/wireguard/selftest/ratelimiter.h:87:
+#define ensure_time do {                                                   \
+               if (time_is_before_jiffies(loop_start_time +               \
+                                          maximum_jiffies_at_index(i))) { \
+                       if (++tries >= 5000)                               \
+                               goto err;                                  \
+                       gc_entries(NULL);                                  \
+                       rcu_barrier();                                     \
+                       msleep(500);                                       \
+                       goto restart;                                      \
+               }                                                          \
+       } while (0)

WARNING: Macros with flow control statements should be avoided
#6948: FILE: drivers/net/wireguard/timers.c:29:
+#define peer_get_from_timer(timer_name)                                        \
+       struct wireguard_peer *peer;                                           \
+       rcu_read_lock_bh();                                                    \
+       peer = peer_get_maybe_zero(from_timer(peer, timer, timer_name));       \
+       rcu_read_unlock_bh();                                                  \
+       if (unlikely(!peer))                                                   \
+               return;

are not very nice.

WARNING: Possible unnecessary 'out of memory' message
#5306: FILE: drivers/net/wireguard/selftest/allowedips.h:261:
+       if (!peers) {
+               pr_info("allowedips random self-test: out of memory\n");

kcalloc and friends are pretty noisy when they fails. No need to add
your own print.

I see we still have a lot of __always_inline in C files :-(

	Andrew