Message-ID: <20201201140449.GG2767@kadam>
Date:   Tue, 1 Dec 2020 17:04:49 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, alsa-devel@...a-project.org,
        linux-atm-general@...ts.sourceforge.net,
        reiserfs-devel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-wireless@...r.kernel.org, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Nathan Chancellor <natechancellor@...il.com>,
        linux-ide@...r.kernel.org, dm-devel@...hat.com,
        keyrings@...r.kernel.org, linux-mtd@...ts.infradead.org,
        GR-everest-linux-l2@...vell.com, wcn36xx@...ts.infradead.org,
        samba-technical@...ts.samba.org, linux-i3c@...ts.infradead.org,
        linux1394-devel@...ts.sourceforge.net,
        linux-afs@...ts.infradead.org,
        usb-storage@...ts.one-eyed-alien.net, drbd-dev@...n.linbit.com,
        devel@...verdev.osuosl.org, linux-cifs@...r.kernel.org,
        rds-devel@....oracle.com,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-scsi@...r.kernel.org, linux-rdma@...r.kernel.org,
        oss-drivers@...ronome.com, bridge@...ts.linux-foundation.org,
        linux-security-module@...r.kernel.org,
        amd-gfx@...ts.freedesktop.org,
        linux-stm32@...md-mailman.stormreply.com, cluster-devel@...hat.com,
        linux-acpi@...r.kernel.org, coreteam@...filter.org,
        intel-wired-lan@...ts.osuosl.org, linux-input@...r.kernel.org,
        Miguel Ojeda <ojeda@...nel.org>,
        tipc-discussion@...ts.sourceforge.net, linux-ext4@...r.kernel.org,
        linux-media@...r.kernel.org, linux-watchdog@...r.kernel.org,
        selinux@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, linux-geode@...ts.infradead.org,
        linux-can@...r.kernel.org, linux-block@...r.kernel.org,
        linux-gpio@...r.kernel.org, op-tee@...ts.trustedfirmware.org,
        linux-mediatek@...ts.infradead.org, xen-devel@...ts.xenproject.org,
        nouveau@...ts.freedesktop.org, linux-hams@...r.kernel.org,
        ceph-devel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-arm-kernel@...ts.infradead.org, linux-hwmon@...r.kernel.org,
        x86@...nel.org, linux-nfs@...r.kernel.org,
        GR-Linux-NIC-Dev@...vell.com, linux-mm@...ck.org,
        netdev@...r.kernel.org, linux-decnet-user@...ts.sourceforge.net,
        linux-mmc@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-renesas-soc@...r.kernel.org, linux-sctp@...r.kernel.org,
        linux-usb@...r.kernel.org, netfilter-devel@...r.kernel.org,
        linux-crypto@...r.kernel.org, patches@...nsource.cirrus.com,
        Joe Perches <joe@...ches.com>, linux-integrity@...r.kernel.org,
        target-devel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 000/141] Fix fall-through warnings for Clang

On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:  
> > > > > This series aims to fix almost all remaining fall-through warnings in
> > > > > order to enable -Wimplicit-fallthrough for Clang.
> > > > > 
> > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
> > > > > add multiple break/goto/return/fallthrough statements instead of just
> > > > > letting the code fall through to the next case.
> > > > > 
> > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this
> > > > > change[1] is meant to be reverted at some point. So, this patch helps
> > > > > to move in that direction.
> > > > > 
> > > > > Something important to mention is that there is currently a discrepancy
> > > > > between GCC and Clang when dealing with switch fall-through to empty case
> > > > > statements or to cases that only contain a break/continue/return
> > > > > statement[2][3][4].  
> > > > 
> > > > Are we sure we want to make this change? Was it discussed before?
> > > > 
> > > > Are there any bugs Clangs puritanical definition of fallthrough helped
> > > > find?
> > > > 
> > > > IMVHO compiler warnings are supposed to warn about issues that could
> > > > be bugs. Falling through to default: break; can hardly be a bug?!  
> > > 
> > > It's certainly a place where the intent is not always clear. I think
> > > this makes all the cases unambiguous, and doesn't impact the machine
> > > code, since the compiler will happily optimize away any behavioral
> > > redundancy.
> > 
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
> 
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/

This is a fallthrough to a return and not to a break.  That should
trigger a warning.  The fallthrough to a break should not generate a
warning.

The bug we're trying to fix is "missing break statement" but if the
result of the bug is "we hit a break statement" then now we're just
talking about style.  GCC should limit itself to warning about
potentially buggy code.

regards,
dan carpenter

Powered by blists - more mailing lists