Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support

[Ingo Molnar]

* Kees Cook <keescook@xxxxxxxxxxxx> 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