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

[Jan Kara <jack@suse.cz>]

Quoting full email for Matthew and Zhengbin to have context.

On Sat 21-09-19 14:10:52, Linus Torvalds wrote:
> 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.

Yeah, I was also a bit undecided about this patch because the check with
"len < 0" seems more obvious. But then decided to take it because we have a
very similar WARN_ON_ONCE() at the beginning of the function
(copy_fid_to_user()) making sure "len" is large enough. But seeing your
arguments I'll just drop the patch. Thanks for review!
 
> 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.

The report has come from a CI system run at Huawei. Not sure what exactly
they run there.

> 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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR