netdev - Re: [PATCH v2 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]

Date:   Sat, 25 Aug 2018 01:00:45 +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, Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2 17/17] net: WireGuard secure network tunnel

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