Re: [PATCH 00/42] x86: updated patches for kaslr and setup_data etc for v4.3

From: Ingo Molnar
Date: Wed Jul 08 2015 - 06:51:57 EST



* Yinghai Lu <yinghai@xxxxxxxxxx> wrote:

> Those patches are rebased on v4.2-rc1 that I sent before but were rejected by
> Ingo on changelog.

What does 'on changelog' mean?

So I rejected them because the patches had poor organization and poor
explanations. I just re-checked this series and the organization is still poor,
and it has similar problems which I pointed out 3 months ago - which need to be
fixed. (Due to that I couldn't even begin to evaluate any deeper merits (or
problems) of the changes.)

> Kees Cook said that he would like to give a try to make improvement on changelog
> to get things moving.

So if that means that Kees is willing to write proper changelogs, then in the mail
below I'm pointing out the most glaring problem with your patches, but there are
other details missing as well - as usual I have to (sadly) say.

But if Kees sends truly new changelogs that meet the technical requirements below
I'll have a look - but the simple Acked-by's from Kees in this thread won't
suffice to fix the deficiencies.


Thanks,

Ingo

=========================================>

In your changelogs you are typically only talking only about the
low level code and about low level symptoms, while in contrast to
that the primary question with _any_ change to the kernel is always:

Why are we changing the kernel, what bad high level behavior can a
user observe if the bug or problem is not fixed?

Your descriptions totally ignore the high level effects of the bug on
the system and on users of the machine, and you fail to describe them
properly. You totally concentrate on the low level: your descriptions
are missing the forest from all the trees.

That makes 90% of your patch descriptions useless.

In fact, 90% of your patches had that deficiency and had it for the
past 4 years, non-stop, and maintainers were complaining to you about
that, non-stop as well. Do you think maintainers enjoy complaining
about deficiencies? Do you wonder why maintainers were forced to
simply stop looking at any complex series from yours after you refused
to change?

> will refresh the patchset.

So let me try this again, one very last time.

That sentence demonstrates your problem: it's not a 'refresh' your
patches need, but a 'hard reboot', a totally new viewpoint that
concentrates on what matters: that zooms out of the small details of
the patch!

For any serious change to the Linux kernel, analyzing small details is
fine and required as well, AFTER the high level has been discussed
properly:

- What happened, what high level concern motivates you to change the
Linux kernel?

And no, starting a changelog with:

commit e6023367d779 ("x86, kaslr: Prevent .bss from
overlaping initrd") introduced one run_size for kaslr.

is not 'high level' in any way, it talks about code in the
first sentence! Talking about code, not talking about high
level concerns is a BUG().

- What was the previous (often bad) high level behavior of the
kernel?

And no, 'KASLR will not find a proper area' is NOT a high level
description, it's a very low level description! Not discussing
high level behavior of the kernel in a changelog is a BUG().

- What new high level behavior do we want to happen instead?

And no, saying that 'KASLR should be passed init_size instead
of run_size' is not a description of desired new high level
behavior! Not discussing the desired high level behavior of the
kernel in a changelog is a BUG().

- What was the high level design of the old code, was that design
fine, should it be changed, and if yes, in what way?

Note that we describe the high level design even if we don't
change it, partly to give context to the reader, partly to
prove that you know what you are doing, to build trust in your
patch! Not discussing the old (and new) design of that area of
the kernel is a BUG().

and only then do we:

- Describe the old implementation, and describe how the new
implementation works in all that context.

Here is where 99.9% of your existing changelogs fit in.
Put differently: your changelogs are missing the first 3
essential components of a changelog.

And note, mentioning 'run_size' in a low level description is
fine. Mentioning it in a high level description is a BUG():
that is why Boris was trying to change your changelogs into
spoken English, to recover some of that missing high level
context. Maintainers like Boris should not be forced to do
that, you are supposed to offer this with any significant
patch, as a passport to prove that your changes to the code are
worth looking at.

And yes, we do it in that order. Take a look at a recent single line
change Linus made in 53da3bc2ba9e48, attached to this mail.

Check how the changelog is structured: it discusses high level
concepts first. It's a _ONE LINER FIX_ from Linus, yet it's spread
over 8 paragraphs and 50 lines, because the change justified that kind
of description.

And observe the moment when Linus, in his 8 paragraphs, 50 lines
description starts talking about low level implementational details,
when he mentions lines of code, function names, such as
do_numa_page(), 'pte_write()' or 'pte_dirty()'.

He doesnt!

It's not needed for a one-liner most of the time: but the high level
concepts around that code are very much necessary to convey.

Now contrast that with your changelogs: your changelogs are stock full
of non-English phrases resembling code more than a fluid description,
they are stock full of low level details, mentioning of function
names, variables and fields with no high level context conveyed
whatsoever.

Let me try to put it to you in a way that might come across: when I
run maintainer code over your changelogs it runs into an instant BUG()
most of the time, forcing me to drop your patches.

High level discussion of old behavior, new behavior, old design and
new design isn't just some detail to be slapped on a change after the
fact, this is a serious and required technological aspect of the Linux
kernel, and 90% of your patches are buggy in that respect.

In fact, I noticed that both your descriptions and your followups to
them are totally lacking the high level viewpoint!

Either you:

- refuse to think in high level concepts when you are writing
patches, in which case we need to keep your patches away from
the Linux kernel,

- or you are unwilling to document such high level thinking
processes, in which case we need to keep your patches away from
the Linux kernel as well.

If your appoach to writing kernel patches does not change then I will
be forced to take action, currently you are this -->.<-- close to
being filtered out from my default inbox for your total refusal to
change the technology of how you are writing patches.

Thanks,

Ingo

[ Sample 'good' changelog from Linus: ]

======================>