Re: [GIT PULL] fanotify cleanup for v5.4-rc1

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Sep 20, 2019 at 4:00 AM Jan Kara <jack@suse.cz> wrote:
>
>   could you please pull from

Pulled and then unpulled.

This is a prime example of a "cleanup" that should never ever be done,
and a compiler warning that is a disgrace and shouldn't happen.

This code:

        WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);

is obvious and makes sense. It clearly and unambiguously checks that
'len' is in the specified range.

In contrast, this code:

        WARN_ON_ONCE(len >= FANOTIFY_EVENT_ALIGN);

quite naturally will make a human wonder "what about negative values".

Yes, it turns out that "len" is unsigned.  That isn't actually
immediately obvious to a human, since the declaration of 'len' is 20+
lines earlier (and even then the type doesn't say "unsigned", although
a lot of people do recognize "size_t" as such).

In fact,  maybe some day the type will change, and the careful range
checking means that the code continues to work correctly.

The fact that "len" is unsigned _is_ obvious to the compiler, which
just means that now that compiler can ignore the "< 0" thing and
optimize it away. Great.

But that doesn't make the compiler warning valid, and it doesn't make
the patch any better.

When it comes to actual code quality, the version that checks against
zero is the better version.

Please stop using -Wtype-limits with compilers that are too stupid to
understand that range checking with the type range is sane.

Compilers that think that warning for the above kind of thing is ok
are inexcusable garbage.

And compiler writers who think that the warning is a good thing can't
see the forest for the trees. They are too hung up on a detail to see
the big picture.

Why/how was this found in the first place? We don't enable type-limit
checking by default.

We may have to add an explicit

   ccflags-y += $(call cc-disable-warning, type-limits)

if these kinds of patches continue to happen, which would be sad.
There are _valid_ type limits.

But this kind of range-based check is not a valid thing to warn about,
and we shouldn't make the kernel source code worse just because the
compiler is doing garbage things.

              Linus