linux-kernel - Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu

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 linux-cve-announce 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:   Wed, 20 Mar 2019 10:19:48 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     "Koenig, Christian" <Christian.Koenig@....com>
Cc:     Nathan Chancellor <natechancellor@...il.com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>
Subject: Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c

On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian
<Christian.Koenig@....com> wrote:
>
> Am 20.03.19 um 05:34 schrieb Nathan Chancellor:
> > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote:
> >> these two enumerated types are same for now. both of them might change in the future.

Please consider if the YAGNI principle applies here.
https://martinfowler.com/bliki/Yagni.html

> >>
> >> I have not used clang, but would  .block_id =  (int)head->block fix your warning? If such change is acceptable, I can make one then.

My preference on solutions, in order:
1. One enum (this is the simplest most type safe solution).  Add
another enum when you actually need it.  Otherwise, YAGNI.
2. Safe casting function (like the one Nathan supplied, maybe with
WARN_ONCE rather than WARN).  This ensures that at least if the types
diverge you get a runtime warning.  A compile-time warning would be
preferred, but I haven't taken the time to think through how that
might be implemented.
3. Cast to int (this has been used in other places throughout the
kernel, but provides the weakest type safety and doesn't catch future
divergence).
4. Disabling the warning. (I almost never advocate for this).

> Another question is if it is such a good idea to just silence the warning?

For the kernel, it seems that each maintainer can choose what to apply
to their subsystem.  I would recommend against disabling additional
warnings that aren't disable kernel-wide for most cases.
-Wenum-conversion has spotted many bugs.  While the enums in question
today are not different, they MIGHT eventually diverge and lead to
bugs, like the others we've found and fixed throughout the kernel.  So
I would recommend fixing now, and be insulated in the future.

-- 
Thanks,
~Nick Desaulniers