Message-ID: <20180903135636.GL19965@ZenIV.linux.org.uk>
Date:   Mon, 3 Sep 2018 14:56:36 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Mark Rutland <mark.rutland@....com>,
        Robin Murphy <robin.murphy@....com>,
        Kees Cook <keescook@...omium.org>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Shuah Khan <shuah@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-doc@...r.kernel.org,
        Linux Memory Management List <linux-mm@...ck.org>,
        linux-arch@...r.kernel.org, linux-kselftest@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        Evgeniy Stepanov <eugenis@...gle.com>,
        Lee Smith <Lee.Smith@....com>,
        Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
        Jacob Bramley <Jacob.Bramley@....com>,
        Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
        Chintan Pandya <cpandya@...eaurora.org>
Subject: Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by
 sparse

On Mon, Sep 03, 2018 at 02:34:27PM +0200, Andrey Konovalov wrote:

> > Al, very annoyed by that kind of information-hiding crap...
> 
> This patch only adds __force to hide the reports I've looked at and
> decided that the code does the right thing. The cases where this is
> not the case are handled by the previous patches in the patchset. I'll
> this to the patch description as well. Is that OK?

I don't know about you, but personally I've run into "I've looked,
I'm sure it's OK here" -> (a year or so later) "why is it OK, again?
Oh, bugger..." quite a few times.  Some, but not all, hadn't been
OK all along, some used to be but got quietly broken by subsequent
changes by people who had no idea that some property of implementation
was critical for correctness in the place in question, some (even
more embarrassingly) were broken by a patch of my own.

It happens.  "Looked in there, decided that the warning was bogus and
quietly shut it up" has turned out to be a source of trouble down the
road a lot of times.  If you are forcibly removing a warning (not by
reorganizing the logics and annotations, that is - by force-cast, or
something similar to it), leave behind something more useful than
"On $DATE I've decided it was OK" (and even that - only accessible via
git blame/git show).  As a hint for yourself, if nothing else - you
might end up asking yourself the same question a year or two (or twenty,
for that matter) later while looking for likely source of odd breakage
and trying to narrow the search down.

Force-cast conflates a *lot* of situations together - it's pretty much
"fuck off, I know what's going on here, it's OK" and no more than that;
hell, even the warning it removes would've carried more information...

That kind of "these are false positives, let's turn them off to search for
real problems" patches is fine when developing a branch like that; it's
leaving them in for posterity that tends to cause PITA...

I'm not attacking you, BTW - it's really a generic point re force-casts.
There had been some really outrageous cases lately[1] and I think that
this point does need to be made.  Unexplained force-cast is worse than
leaving a warning in.

[1] with, if my reading of the situation is correct,
	newbie asking maintainers if dealing with endianness warnings in
a certain driver would be useful
	newbie getting told (perhaps by maintainers, perhaps by somebody else)
that those were all noise, the driver's correct and the most useful thing to
be done with them would be to make them STFU
	force-cast-laden patch from said newbie doing just that picked by said
maintainers, "cleaning up" the warnings.  And committed with authorship pinned
to the newbie ;-/
Not nice, seeing that the code in driver is *not* correct, despite the high-handed
"shut that noise off, everything's fine there" commit - undoing that "cleanup" and
trying to redo annotations properly starts to converge on absolutely real bugs on
b-e hosts in about 10 minutes.  In PCIe driver, with devices existing as separate
cards, not just something always embedded into x86 or arm motherboard...

Powered by blists - more mailing lists