[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 13 Apr 2016 12:19:50 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Kees Cook <keescook@...omium.org>
Cc: Ingo Molnar <mingo@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
Baoquan He <bhe@...hat.com>, Yinghai Lu <yinghai@...nel.org>,
"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
Vivek Goyal <vgoyal@...hat.com>, Andy Lutomirski <luto@...nel.org>,
lasse.collin@...aani.org, Andrew Morton <akpm@...ux-foundation.org>,
Dave Young <dyoung@...hat.com>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support
* Kees Cook <keescook@...omium.org> wrote:
> FWIW, I've also had this tree up in my git branches, and the 0day
> tester hasn't complained at all about it in the last two weeks. I'd
> really like to see this in -next to fix the >4G (mainly kexec) issues
> and get us to feature parity with the arm64 kASLR work (randomized
> virtual address).
So I started applying the patches, started fixing up changelogs and gave up on
patch #3.
Changelogs of such non-trivial code need to be proper English and need to be
understandable.
For example patch #3 starts with:
> Current z_extract_offset is calculated in boot/compressed/mkpiggy.c. The problem
> is in mkpiggy.c we don't know the detail of decompressor. Then we just get a
> rough z_extract_offset according to extra_bytes. As we know extra_bytes can only
> promise a safety margin when decompressing. In fact this brings some risks:
Beyond the bad grammar of the _first word_ of the changelog, this is not a proper
high level description of the change. A _real_ high level description would be
something like:
> Currently z_extract_offset is calculated during kernel build time. The problem
> with that method is that at this stage we don't yet know the decompression
> buffer sizes - we only know that during bootup.
>
> Effects of this are that when we calculate z_extract_offset during the build
> we don't know the precise decompression details, we'll only get a rough
> estimation of z_extract_offset.
>
> Instead of that we want to calculate it during bootup.
etc. etc. - the whole series is _full_ of such crappy changelogs that make it
difficult for me and others to see whether the author actually _understands_ the
existing code or is hacking away on it. It's also much harder to review and
validate.
This is totally unacceptable.
Please make sure every changelog starts with a proper high level description that
tells the story and convinces the reader about what the problem is and what the
change should be.
And part of that are the patch titles. Things like:
Subject: [PATCH v3 03/19] x86, boot: Move z_extract_offset calculation to header.S
are absolutely mindless titles. A better title would be:
x86/boot: Calculate precise decompressor parameters during bootup, not build time
... or something like that. Even having read the changelog 3 times I'm unsure what
the change really is about.
Thanks,
Ingo
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
