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  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]
Date:   Sun, 26 Nov 2017 10:38:55 -0700
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Joe Perches <joe@...ches.com>, Julia Lawall <julia.lawall@...6.fr>
Cc:     linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Andy Whitcroft <apw@...onical.com>
Subject: Re: [PATCH v2] checkpatch: Add a warning for log messages that don't
 end in a new line



On 25/11/17 11:01 PM, Joe Perches wrote:
> It doesn't really work.

That's rather hyperbolic and I don't appreciate the tone.

> Many of the messages aren't missing newlines.
> 
> I only looked a the first few dozen instances, but many of
> them aren't really missing newlines, but are now missing a
> KERN_CONT annotation.

True, and I mentioned that. However, these instances are still incorrect
and deserve a warning. I don't see how any tool (even one written in
Coccinelle) could determine whether the programmer intended to have a
new line or intended for the next line to be a continuation. But if the
developer gets a warning they'd at least look at it. Given that
KERN_CONT usage is discouraged, I think warning that a new line is
required is acceptable.

> Most of that commit message is BS, but the net effect is
> that now printks must have a KERN_<LEVEL> marker or a
> newline is inserted before the format.

Yes, that just means that all the instances that should be using
KERN_CONT are now *actually* broken. It's also part of what created the
mess in the first place as it makes the final new line seem to not be
required. As a result, there are now plenty of places in the kernel that
are inconsistent. That's why this patch is needed.

> Also, this patch logic will be very confused by patch
> blocks and not files.

No, it's not. (With the exception of the false positives I noted due to
KERN_CONTs not making it into the patch context.) I've tested this. I
even created some pathological tests[1] to ensure crossing hunk
boundaries and other weirdness works correctly. There may also be false
negatives in extreme cases if the entire call site doesn't fit in the
patch context but it's still better than nothing and will catch all
newly added calls. So if you're going to make such statements I suggest
you back it up with evidence.

Logan

[1]
https://github.com/lsgunth/checkpatch-tests/blob/master/examples/test2.patch

Powered by blists - more mailing lists