linux-kernel - Re: [PATCH v1 3/3] 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: <20180803143936.GD27445@lunn.ch>
Date:   Fri, 3 Aug 2018 16:39:36 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v1 3/3] net: WireGuard secure network tunnel

On Fri, Aug 03, 2018 at 02:35:40AM +0200, Jason A. Donenfeld wrote:
> On Tue, Jul 31, 2018 at 10:02 PM Andrew Lunn <andrew@...n.ch> wrote:
> > I just gave this patch to checkpatch.pl...
> On Tue, Jul 31, 2018 at 10:22 PM Stephen Hemminger
> <stephen@...workplumber.org> wrote:
> > Please break lines at something reasonable like 100 characters.
> 
> If the long lines really truly are dreadful, I have no problem fixing
> that up for v2.

A very large percentage of the kernel code uses 80 characters maximum.
So anything which does not makes it look different. It makes it not
look like kernel code. It is also not just about styling. If you use
long lines, it makes it easier to use more indentation levels. But
good readable code tends to have lots of little functions with only
one or two levels of indentation, and each function does just one
easily understood thing.

Plus this is pretty much an open invite for kernel newbies to submit
patches fixing up all these warnings. Maybe you would prefer to do it
yourself, so you keep some degree of control over it. Also, such white
space changes make backporting fixes more difficult. So it is better
if you can get this done before it lands in the kernel and backporting
becomes necessary.

> On Tue, Jul 31, 2018 at 10:02 PM Andrew Lunn <andrew@...n.ch> wrote:
> > > +static __always_inline void swap_endian(u8 *dst, const u8 *src, u8 bits)
> > There is a general preference to not force the compile to
> > inline. Leave it to decide.
> 
> I'm aware this is for the most part the case, and I've read the
> variety of threads and documentation of folks explaining why this is a
> good policy. In the particular instance of that function, inlining is
> in fact always the right thing to do. But I'll give it a double check
> to see if the compiler is already figuring that out on its own.

Well, what compiler are you going to check? I think the minimum
version of GCC is now v4.5. So you probably want to check 8.2, 7.3,
6.4, 5.5, 4.9.3 and 4.8. And you might want to check ARM, MIPs, x86
and amd64. So only 24 versions.

More realistically, if you have a strong argument why a particular
function must be inlined, e.g. to avoid a timing attack, make that
argument.

	Andrew