linux-kernel - Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

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:   Tue, 26 Jun 2018 19:43:16 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Cc:     Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        David Airlie <airlied@...ux.ie>,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

Hi Jani,

On 06/21/2018 03:03 AM, Jani Nikula wrote:
> On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@...eddedor.com> wrote:
>> On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
>>> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>> where we are expecting to fall through.
>>>>
>>>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
>>>
>>> Any other advantage besides coverity?
>>> Can't we address it by marking as "Intentional" on the tool?
>>>
>>
>> Yes. The advantage of this is that it will eventually allows to enable 
>> -Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
>> warning, which will force us to double check if we are actually missing 
>> a break before committing the code.
> 
> I applaud the efforts. Since you're doing the comment changes, do you
> have an idea what -Wimplicit-fallthrough=N level is being considered for
> kernel?
> 

Currently, we are trying level 2.

>>> I'm afraid there will be so many more places to add fallthrough
>>> marks....
>>>
>>
>> Oh yeah, there are around 1000 similar places in the whole codebase. 
>> There is an ongoing effort to review each case. Months ago, it used to 
>> be around 1500 of these cases.
> 
> We use our own MISSING_CASE() to indicate stuff that's not supposed to
> happen, or to be implemented, etc. and in many cases the fallthrough is
> normal. I wonder if we could embed __attribute__ ((fallthrough)) in
> there to tackle all of these without a comment.
> 

I've tried this:

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 00165ad..829572c 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -40,8 +40,10 @@
 #undef WARN_ON_ONCE
 #define WARN_ON_ONCE(x) WARN_ONCE((x), "%s", "WARN_ON_ONCE(" __stringify(x) ")")

-#define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
-                            __stringify(x), (long)(x))
+#define MISSING_CASE(x) ({ \
+       WARN(1, "Missing case (%s == %ld)\n", __stringify(x), (long)(x)); \
+       __attribute__ ((fallthrough)); \
+})

 #if GCC_VERSION >= 70000
 #define add_overflows(A, B) \

and I get the following warnings as a consequence:

drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_init_clock_gating_hooks’:
drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
  __attribute__ ((fallthrough)); \
  ^
drivers/gpu/drm/i915/intel_pm.c:9240:3: note: in expansion of macro ‘MISSING_CASE’
   MISSING_CASE(INTEL_DEVID(dev_priv));
   ^~~~~~~~~~~~
drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_read_wm_latency’:
drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
  __attribute__ ((fallthrough)); \
  ^
drivers/gpu/drm/i915/intel_pm.c:2902:3: note: in expansion of macro ‘MISSING_CASE’
   MISSING_CASE(INTEL_DEVID(dev_priv));
   ^~~~~~~~~~~~

Thanks
--
Gustavo