lkml.org 
[lkml]   [2015]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly

    * Yinghai Lu <yinghai@kernel.org> wrote:

    > On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov <bp@alien8.de> wrote:
    > > On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
    > >> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
    > >> is using address as value for kaslr_enabled.
    > >>
    > >> That will random kaslr_enabled get that set or cleared.
    > >> Will have problem for system really have kaslr enabled.
    > >>
    > >> -v2: update changelog.
    > >
    > > This is still not good enough. Please do this:
    > >
    > > In commit f47233c2d34f we did A. The problem with that is B. Change the
    > > code to do C.
    > >
    > > Now you only have to fill out the A,B and C variables with the
    > > respective text which is understandable even for people who don't know
    > > this code.
    > >
    >
    > I don't know, that is trivial and obvious.

    The fix might be obvious, the effects of the bug are not obvious at
    all, as you yourself show that you don't understand your own change,
    which is evident from the changelog you've written:

    > Please check if it is ok:
    >
    > Subject: [PATCH v3] x86, kaslr: get kaslr_enabled back correctly

    Missing capitalization.

    >
    > commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
    > is using address as value for kaslr_enabled.

    Missing capitalization. Do you really expect maintainers to fix up
    every single sentence of yours??

    > That will get wrong value for kaslr_enabled, so have problem for
    > system really have kaslr enabled.

    This sentence does not parse, nor is it correct: the bug isn't just
    triggering on systems that want to have kaslr enabled - but also on
    bootloaders that happen to pass in a kaslr boot parameter but have the
    switch value disabled...

    You also need to point out the important fact that bootloaders that
    don't try to use the kaslr extension (i.e. that don't use SETUP_KASLR)
    work just fine - this is why the bug was not noticed to begin with,
    i.e. the overwhelming majority of systems out there.

    > This patch change to using early map and accessing the value.

    s/change to using/changes the code to use/

    It is totally unacceptable that you don't do proper analysis of the
    patches you submit, and that you don't bother writing proper, readable
    changelogs.

    Your flippant "that is trivial and obvious" attitude towards
    changelogs is unacceptable as well. And this is not about English
    knowledge: missing capitalization is a very simple concept any
    beginning coder should be able to graps the first time it's pointed
    out... yet for the past 3 years half of your patches had totally
    careless, often unreadable changelogs.

    These subpar changelogs and patches show plain laziness, sloppiness
    and lack of care to write clean changelogs - and that sloppiness not
    only makes it much harder for maintainers to process your patches, but
    also tends to creep over into your patches as well, causing repeat
    problems again and again...

    Thanks,

    Ingo


    \
     
     \ /
      Last update: 2015-03-04 21:21    [W:4.124 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site