Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 27 May 2021 14:31:12 -0700
From:   Kees Cook <keescook@chromium.org>
To:     "Darrick J. Wong" <djwong@kernel.org>
Cc:     "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org, joe@perches.com
Subject: Re: [PATCH][next] xfs: Fix fall-through warnings for Clang
Message-ID: <202105271358.22E0E2BFD@keescook>
References: <20210420230652.GA70650@embeddedor>
 <20210420233850.GQ3122264@magnolia>
 <62895e8c-800d-fa7b-15f6-480179d552be@embeddedor.com>
 <bcae9d46-644c-d6f6-3df5-e8f7c50a673d@embeddedor.com>
 <20210526211624.GB202121@locust>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210526211624.GB202121@locust>
Precedence: bulk

On Wed, May 26, 2021 at 02:16:24PM -0700, Darrick J. Wong wrote:
> On Wed, May 26, 2021 at 01:21:06PM -0500, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 4/20/21 18:56, Gustavo A. R. Silva wrote:
> > > 
> > > 
> > > On 4/20/21 18:38, Darrick J. Wong wrote:
> > >> On Tue, Apr 20, 2021 at 06:06:52PM -0500, Gustavo A. R. Silva wrote:
> > >>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > >>> the following warnings by replacing /* fall through */ comments,
> > >>> and its variants, with the new pseudo-keyword macro fallthrough:
> > >>>
> > >>> fs/xfs/libxfs/xfs_alloc.c:3167:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_da_btree.c:286:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_ag_resv.c:346:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/libxfs/xfs_ag_resv.c:388:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_bmap_util.c:246:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_export.c:88:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_export.c:96:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_file.c:867:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_ioctl.c:562:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_ioctl.c:1548:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_iomap.c:1040:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_inode.c:852:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_log.c:2627:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/xfs_trans_buf.c:298:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/bmap.c:275:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/btree.c:48:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:85:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:138:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/common.c:698:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/dabtree.c:51:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>> fs/xfs/scrub/repair.c:951:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> > >>>
> > >>> Notice that Clang doesn't recognize /* fall through */ comments as
> > >>> implicit fall-through markings, so in order to globally enable
> > >>> -Wimplicit-fallthrough for Clang, these comments need to be
> > >>> replaced with fallthrough; in the whole codebase.
> > >>>
> > >>> Link: https://github.com/KSPP/linux/issues/115
> > >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > >>
> > >> I've already NAKd this twice, so I guess I'll NAK it a third time.
> > > 
> > > Darrick,
> > > 
> > > The adoption of fallthrough; has been already accepted and in use since Linux v5.7:
> > > 
> > > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > > 
> > > This change is needed, and I would really prefer if this goes upstream through your tree.
> > > 
> > > Linus has taken these patches directly for a while, now.
> > > 
> > > Could you consider taking it this time? :)
> > > 
> > 
> > Hi Darrick,
> > 
> > If you don't mind, I will take this in my -next[1] branch for v5.14, so we can globally enable
> > -Wimplicit-fallthrough for Clang in that release.
> > 
> > We had thousands of these warnings and now we are down to 47 in next-20210526,
> > 22 of which are fixed with this patch.
> 
> I guess we're all required to kowtow to a bunch of effing bots now.
> Hooray for having to have a macro to code-switch for the sake of
> stupid compiler writers who refuse to give the rest of us a single
> workable way to signal "this switch code block should not end here":
> 
> /* fall through */
> __attribute__((fallthrough));
> do { } while (0) /* fall through */
> 
> and soon the ISO geniuses will make it worse by adding to C2x:
> 
> [[fallthrough]];
> 
> Hooray!  Macros to abstractify stupidity!!!!
> 
> Dave and I have told you and Miaohe several[1] times[2] to fix[3] the
> compiler, but clearly you don't care what we think and have decided to
> ram this in through Linus anyway.

To clarify, we certainly _do_ care what you think. It's just that
when faced with the difficulties of the compiler's implementations of
handling this, the kernel had to get creative and pick the least-bad of
many bad choices. We're trying to make the kernel safer for everyone,
and this particular C language weakness has caused us a significant
number of bugs. Eradicating it is worth the effort.

All that said, as you pointed out, you _have_ asked before[1] to just
have Linus take it without bothering you directly, so okay, that can be
done. Generally maintainers have wanted these changes to go through their
trees so it doesn't cause them merge pain, but it seems you'd prefer it
the other way around.

-Kees

[1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
"If you feel really passionate about ramming a bunch of pointless churn
 into the kernel tree to make my life more painful, send this to Linus
 and let him make the change."

> Since that is what you choose, do not send me email again.
> 
> NAKed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> [1] https://lore.kernel.org/linux-xfs/20200820191237.GK6096@magnolia/
> [2] https://lore.kernel.org/linux-xfs/20210420230652.GA70650@embeddedor/
> [3] https://lore.kernel.org/linux-xfs/20200708065512.GN2005@dread.disaster.area/
> 
> > 
> > Thanks
> > --
> > Gustavo
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp

-- 
Kees Cook