[PATCH] Makefile: Globally enable fall-through warning

[PATCH] Makefile: Globally enable fall-through warning

[ALOK JHA]

From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>

Now that all the fall-through warnings have been addressed in the
kernel, enable the fall-through warning globally.

Also, update the deprecated.rst file to include implicit fall-through
as 'deprecated' so people can be pointed to a single location for
justification.

Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Kees Cook <keescook@chromium.org>
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 Documentation/process/deprecated.rst | 14 ++++++++++++++
 Makefile                             |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64a3427..053b24a6dd38 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -119,3 +119,17 @@ array may exceed the remaining memory in the stack segment. This could
 lead to a crash, possible overwriting sensitive contents at the end of the
 stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting
 memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`)
+
+Implicit switch case fall-through
+---------------------------------
+The C language allows switch cases to "fall through" when
+a "break" statement is missing at the end of a case. This,
+however, introduces ambiguity in the code, as it's not always
+clear if the missing break is intentional or a bug. As there
+have been a long list of flaws `due to missing "break" statements
+<https://cwe.mitre.org/data/definitions/484.html>`_, we no longer allow
+"implicit fall-through". In order to identify an intentional fall-through
+case, we have adopted the marking used by static analyzers: a comment
+saying `/* Fall through */`. Once the C++17 `__attribute__((fallthrough))`
+is more widely handled by C compilers, static analyzers, and IDEs, we can
+switch to using that instead.
diff --git a/Makefile b/Makefile
index 9be5834073f8..bdf8eac51b07 100644
--- a/Makefile
+++ b/Makefile
@@ -843,6 +843,9 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 # warn about C99 declaration after statement
 KBUILD_CFLAGS += -Wdeclaration-after-statement
 
+# Warn about unmarked fall-throughs in switch statement.
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
+
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla
 
-- 
2.34.1

Re: [PATCH] Makefile: Globally enable fall-through warning

[Andrew Morton]

On Tue, 17 May 2022 23:05:34 +0530 ALOK JHA <alok08jha@gmail.com> wrote:

> From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> 
> Now that all the fall-through warnings have been addressed in the
> kernel, enable the fall-through warning globally.
> 
> Also, update the deprecated.rst file to include implicit fall-through
> as 'deprecated' so people can be pointed to a single location for
> justification.
> 
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: linux-kbuild@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Since you sent the patch, it should have your signoff as well as
Gustavo's.  Please resend?

Re: [PATCH] Makefile: Globally enable fall-through warning

[Andrew Morton]

On Tue, 17 May 2022 23:05:34 +0530 ALOK JHA <alok08jha@gmail.com> wrote:

> From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> 
> Now that all the fall-through warnings have been addressed in the
> kernel, enable the fall-through warning globally.
> 
> Also, update the deprecated.rst file to include implicit fall-through
> as 'deprecated' so people can be pointed to a single location for
> justification.
> 
> ...
>
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -119,3 +119,17 @@ array may exceed the remaining memory in the stack segment. This could
>  lead to a crash, possible overwriting sensitive contents at the end of the
>  stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting
>  memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`)
> +
> +Implicit switch case fall-through
> +---------------------------------
>
> ...
>

Documentation/process/deprecated.rst already has a section "Implicit
switch case fall-through".  Maybe you're working against an old kernel.
Please update when resending.

Re: [PATCH] Makefile: Globally enable fall-through warning

[Jeff Johnson]

On 5/17/2022 11:46 AM, Andrew Morton wrote:
> On Tue, 17 May 2022 23:05:34 +0530 ALOK JHA <alok08jha@gmail.com> wrote:
> 
>> From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
>>
>> Now that all the fall-through warnings have been addressed in the
>> kernel, enable the fall-through warning globally.
>>
>> Also, update the deprecated.rst file to include implicit fall-through
>> as 'deprecated' so people can be pointed to a single location for
>> justification.
>>
>> ...
>>
>> --- a/Documentation/process/deprecated.rst
>> +++ b/Documentation/process/deprecated.rst
>> @@ -119,3 +119,17 @@ array may exceed the remaining memory in the stack segment. This could
>>   lead to a crash, possible overwriting sensitive contents at the end of the
>>   stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting
>>   memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`)
>> +
>> +Implicit switch case fall-through
>> +---------------------------------
>>
>> ...
>>
> 
> Documentation/process/deprecated.rst already has a section "Implicit
> switch case fall-through".  Maybe you're working against an old kernel.
> Please update when resending.
> 

shouldn't we now just be referencing the fallthrough() macro?

Re: [PATCH] Makefile: Globally enable fall-through warning

[Gustavo A. R. Silva]

On Tue, May 17, 2022 at 11:46:01AM -0700, Andrew Morton wrote:
> On Tue, 17 May 2022 23:05:34 +0530 ALOK JHA <alok08jha@gmail.com> wrote:
> 
> > From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> > 
> > Now that all the fall-through warnings have been addressed in the
> > kernel, enable the fall-through warning globally.
> > 
> > Also, update the deprecated.rst file to include implicit fall-through
> > as 'deprecated' so people can be pointed to a single location for
> > justification.
> > 
> > ...
> >
> > --- a/Documentation/process/deprecated.rst
> > +++ b/Documentation/process/deprecated.rst
> > @@ -119,3 +119,17 @@ array may exceed the remaining memory in the stack segment. This could
> >  lead to a crash, possible overwriting sensitive contents at the end of the
> >  stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting
> >  memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`)
> > +
> > +Implicit switch case fall-through
> > +---------------------------------
> >
> > ...
> >
> 
> Documentation/process/deprecated.rst already has a section "Implicit
> switch case fall-through".  Maybe you're working against an old kernel.
> Please update when resending.

This looks like spam to me.

Let's just ignore this.

--
Gustavo

Re: [PATCH] Makefile: Globally enable fall-through warning

[Guenter Roeck]

On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
> Now that all the fall-through warnings have been addressed in the
> kernel, enable the fall-through warning globally.
> 

Not really "all".

powerpc:85xx/sbc8548_defconfig:

arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
arch/powerpc/kernel/align.c:178:8: error: this statement may fall through

Plus many more similar errors in the same file.

All sh builds:

arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through

Again, this is seen in several places.

mips:cavium_octeon_defconfig:

arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
include/linux/device.h:1499:2: error: this statement may fall through

None of those are from recent changes. And this is just from my small
subset of test builds.

Guenter

Re: [PATCH] Makefile: Globally enable fall-through warning

[Gustavo A. R. Silva]

Hi Guenter,

On 7/28/19 8:58 AM, Guenter Roeck wrote:
> On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
>> Now that all the fall-through warnings have been addressed in the
>> kernel, enable the fall-through warning globally.
>>
> 
> Not really "all".
> 
> powerpc:85xx/sbc8548_defconfig:
> 
> arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
> arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
> 
> Plus many more similar errors in the same file.
> 
> All sh builds:
> 
> arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
> arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through
> 
> Again, this is seen in several places.
> 
> mips:cavium_octeon_defconfig:
> 
> arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
> include/linux/device.h:1499:2: error: this statement may fall through
> 
> None of those are from recent changes. And this is just from my small
> subset of test builds.
> 

Thank you for letting me know about this. I don't have access to build
infrastructure like yours.

My build infrastructure is similar to that of Linus.

But if you send me all of those I can create a patch and send it back
to you to make sure what you see is addressed. If we can coordinate for
this it'd be great for everybody. :)

Thanks
--
Gustavo

Re: [PATCH] Makefile: Globally enable fall-through warning

[Guenter Roeck]

Hi Gustavo,

On Sun, Jul 28, 2019 at 11:42:28AM -0500, Gustavo A. R. Silva wrote:
> Hi Guenter,
> 
> On 7/28/19 8:58 AM, Guenter Roeck wrote:
> > On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
> >> Now that all the fall-through warnings have been addressed in the
> >> kernel, enable the fall-through warning globally.
> >>
> > 
> > Not really "all".
> > 
> > powerpc:85xx/sbc8548_defconfig:
> > 
> > arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
> > arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
> > 
> > Plus many more similar errors in the same file.
> > 
> > All sh builds:
> > 
> > arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
> > arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through
> > 
> > Again, this is seen in several places.
> > 
> > mips:cavium_octeon_defconfig:
> > 
> > arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
> > include/linux/device.h:1499:2: error: this statement may fall through
> > 
> > None of those are from recent changes. And this is just from my small
> > subset of test builds.
> > 
> 
> Thank you for letting me know about this. I don't have access to build
> infrastructure like yours.
> 

I am always happy to run test builds on my infrastructure.

> My build infrastructure is similar to that of Linus.
> 
> But if you send me all of those I can create a patch and send it back
> to you to make sure what you see is addressed. If we can coordinate for
> this it'd be great for everybody. :)
> 

Just have a look at the output of https://kerneltests.org/builders/,
in the 'master' and/or 'next' column. There are many additional warnings
in 'next'. Only downside is that you won't see the warnings unless there
are also build errors, but -next tends to have lots of those.

Just wondering ... wouldn't it be possible to run a coccinelle script
to identify those problems automatically, without depending on compile
warnings ? Or smatch/sparse, maybe ?

Thanks,
Guenter

Re: [PATCH] Makefile: Globally enable fall-through warning

[Gustavo A. R. Silva]

On 7/28/19 12:14 PM, Guenter Roeck wrote:
> Hi Gustavo,
> 
> On Sun, Jul 28, 2019 at 11:42:28AM -0500, Gustavo A. R. Silva wrote:
>> Hi Guenter,
>>
>> On 7/28/19 8:58 AM, Guenter Roeck wrote:
>>> On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
>>>> Now that all the fall-through warnings have been addressed in the
>>>> kernel, enable the fall-through warning globally.
>>>>
>>>
>>> Not really "all".
>>>
>>> powerpc:85xx/sbc8548_defconfig:
>>>
>>> arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
>>> arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
>>>
>>> Plus many more similar errors in the same file.
>>>
>>> All sh builds:
>>>
>>> arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
>>> arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through
>>>
>>> Again, this is seen in several places.
>>>
>>> mips:cavium_octeon_defconfig:
>>>
>>> arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
>>> include/linux/device.h:1499:2: error: this statement may fall through
>>>
>>> None of those are from recent changes. And this is just from my small
>>> subset of test builds.
>>>
>>
>> Thank you for letting me know about this. I don't have access to build
>> infrastructure like yours.
>>
> 
> I am always happy to run test builds on my infrastructure.
> 

Thank you!

>> My build infrastructure is similar to that of Linus.
>>
>> But if you send me all of those I can create a patch and send it back
>> to you to make sure what you see is addressed. If we can coordinate for
>> this it'd be great for everybody. :)
>>
> 
> Just have a look at the output of https://kerneltests.org/builders/,
> in the 'master' and/or 'next' column. There are many additional warnings
> in 'next'. Only downside is that you won't see the warnings unless there
> are also build errors, but -next tends to have lots of those.
> 

I see.

mm... for some reason I'm not able to establish connection with that site...

> Just wondering ... wouldn't it be possible to run a coccinelle script
> to identify those problems automatically, without depending on compile
> warnings ? Or smatch/sparse, maybe ?
> 

That was a common question from people along the whole process. The short
answer is: no. The reason for that is that Coccinelle is not a sophisticated
enough tool to determine if we are dealing with a false positive or an actual
bug.

That's why a code audit was needed.

Thanks
--
Gustavo

Re: [PATCH] Makefile: Globally enable fall-through warning

[Guenter Roeck]

On 7/28/19 10:31 AM, Gustavo A. R. Silva wrote:
> 
> 
> On 7/28/19 12:14 PM, Guenter Roeck wrote:
>> Hi Gustavo,
>>
>> On Sun, Jul 28, 2019 at 11:42:28AM -0500, Gustavo A. R. Silva wrote:
>>> Hi Guenter,
>>>
>>> On 7/28/19 8:58 AM, Guenter Roeck wrote:
>>>> On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
>>>>> Now that all the fall-through warnings have been addressed in the
>>>>> kernel, enable the fall-through warning globally.
>>>>>
>>>>
>>>> Not really "all".
>>>>
>>>> powerpc:85xx/sbc8548_defconfig:
>>>>
>>>> arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
>>>> arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
>>>>
>>>> Plus many more similar errors in the same file.
>>>>
>>>> All sh builds:
>>>>
>>>> arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
>>>> arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through
>>>>
>>>> Again, this is seen in several places.
>>>>
>>>> mips:cavium_octeon_defconfig:
>>>>
>>>> arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
>>>> include/linux/device.h:1499:2: error: this statement may fall through
>>>>
>>>> None of those are from recent changes. And this is just from my small
>>>> subset of test builds.
>>>>
>>>
>>> Thank you for letting me know about this. I don't have access to build
>>> infrastructure like yours.
>>>
>>
>> I am always happy to run test builds on my infrastructure.
>>
> 
> Thank you!
> 
>>> My build infrastructure is similar to that of Linus.
>>>
>>> But if you send me all of those I can create a patch and send it back
>>> to you to make sure what you see is addressed. If we can coordinate for
>>> this it'd be great for everybody. :)
>>>
>>
>> Just have a look at the output of https://kerneltests.org/builders/,
>> in the 'master' and/or 'next' column. There are many additional warnings
>> in 'next'. Only downside is that you won't see the warnings unless there
>> are also build errors, but -next tends to have lots of those.
>>
> 
> I see.
> 
> mm... for some reason I'm not able to establish connection with that site...
> 

What is your host's IP address ? It might be auto-filtered; the site is hosted
on my own server and is under constant attack. Any subnet originating a sequence
of attacks will be blocked automatically.

Thanks,
Guenter

>> Just wondering ... wouldn't it be possible to run a coccinelle script
>> to identify those problems automatically, without depending on compile
>> warnings ? Or smatch/sparse, maybe ?
>>
> 
> That was a common question from people along the whole process. The short
> answer is: no. The reason for that is that Coccinelle is not a sophisticated
> enough tool to determine if we are dealing with a false positive or an actual
> bug.
> 
> That's why a code audit was needed.
> 
> Thanks
> --
> Gustavo
> 
> 
> 
> 
> 
> 

Re: [PATCH] Makefile: Globally enable fall-through warning

[Geert Uytterhoeven]

Hi Gustavo,

On Sun, Jul 28, 2019 at 6:44 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> On 7/28/19 8:58 AM, Guenter Roeck wrote:
> > On Thu, Jun 06, 2019 at 07:46:17PM -0500, Gustavo A. R. Silva wrote:
> >> Now that all the fall-through warnings have been addressed in the
> >> kernel, enable the fall-through warning globally.
> >>
> >
> > Not really "all".
> >
> > powerpc:85xx/sbc8548_defconfig:
> >
> > arch/powerpc/kernel/align.c: In function ‘emulate_spe’:
> > arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
> >
> > Plus many more similar errors in the same file.
> >
> > All sh builds:
> >
> > arch/sh/kernel/disassemble.c: In function 'print_sh_insn':
> > arch/sh/kernel/disassemble.c:478:8: error: this statement may fall through
> >
> > Again, this is seen in several places.
> >
> > mips:cavium_octeon_defconfig:
> >
> > arch/mips/cavium-octeon/octeon-usb.c: In function 'dwc3_octeon_clocks_start':
> > include/linux/device.h:1499:2: error: this statement may fall through
> >
> > None of those are from recent changes. And this is just from my small
> > subset of test builds.
> >
>
> Thank you for letting me know about this. I don't have access to build
> infrastructure like yours.
>
> My build infrastructure is similar to that of Linus.
>
> But if you send me all of those I can create a patch and send it back
> to you to make sure what you see is addressed. If we can coordinate for
> this it'd be great for everybody. :)
>

More to be found in
https://lore.kernel.org/lkml/20190729081727.6094-1-geert@linux-m68k.org/

and I saw the following in my local builds (not detected above due to
kisskb using an older compiler for m68k builds):

arch/m68k/include/asm/amigahw.h: warning: this statement may fall
through [-Wimplicit-fallthrough=]:  => 42:50
drivers/block/ataflop.c: warning: this statement may fall through
[-Wimplicit-fallthrough=]:  => 1728:3
drivers/net/arcnet/com20020-isa.c: warning: this statement may fall
through [-Wimplicit-fallthrough=]:  => 205:13, 203:10, 209:7, 201:11,
207:8
drivers/scsi/sun3_scsi.c: warning: this statement may fall through
[-Wimplicit-fallthrough=]:  => 399:9, 403:9
sound/oss/dmasound/dmasound_atari.c: warning: this statement may fall
through [-Wimplicit-fallthrough=]:  => 1449:24

Thanks for fixing ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Makefile: Globally enable fall-through warning

[Gustavo A. R. Silva]

Hi Geert,

On 7/29/19 3:22 AM, Geert Uytterhoeven wrote:

> 
> More to be found in
> https://lore.kernel.org/lkml/20190729081727.6094-1-geert@linux-m68k.org/
> 

I already sent out patches for many of those. :)

Thanks a lot!

> and I saw the following in my local builds (not detected above due to
> kisskb using an older compiler for m68k builds):
> 
> arch/m68k/include/asm/amigahw.h: warning: this statement may fall
> through [-Wimplicit-fallthrough=]:  => 42:50
> drivers/block/ataflop.c: warning: this statement may fall through
> [-Wimplicit-fallthrough=]:  => 1728:3
> drivers/net/arcnet/com20020-isa.c: warning: this statement may fall
> through [-Wimplicit-fallthrough=]:  => 205:13, 203:10, 209:7, 201:11,
> 207:8
> drivers/scsi/sun3_scsi.c: warning: this statement may fall through
> [-Wimplicit-fallthrough=]:  => 399:9, 403:9
> sound/oss/dmasound/dmasound_atari.c: warning: this statement may fall
> through [-Wimplicit-fallthrough=]:  => 1449:24
> 

I already sent patches for some of the ones above, too. Although, I have
some doubts with the one at arch/m68k/amiga/config.c:223

We can talk about it in a separate thread.

> Thanks for fixing ;-)
> 

Glad to help. :)

Thanks
--
Gustavo