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

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

[Jan Kara]

  Hello Linus,

  could you please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify_for_v5.4-rc1

Top of the tree is ba38e358907e. The full shortlog is:

zhengbin (1):
      fanotify: remove always false comparison in copy_fid_to_user

The diffstat is

 fs/notify/fanotify/fanotify_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

							Thanks
								Honza

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

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

[Linus Torvalds]

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

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

[Jan Kara]

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