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
| ||
|
Message-ID: <CAKwvOd=8Vgv-OUxj09zReaXUZEGKsGzHGNzHnr7J8k90Qf0H1A@mail.gmail.com> Date: Mon, 25 Mar 2019 10:29:17 -0700 From: Nick Desaulniers <ndesaulniers@...gle.com> To: Arnd Bergmann <arnd@...db.de> Cc: "Theodore Ts'o" <tytso@....edu>, Andreas Dilger <adilger.kernel@...ger.ca>, clang-built-linux@...glegroups.com, Nathan Chancellor <natechancellor@...il.com>, Eric Whitney <enwlinux@...il.com>, Sean Fu <fxinrong@...il.com>, Jan Kara <jack@...e.cz>, Eric Biggers <ebiggers@...gle.com>, linux-ext4@...r.kernel.org, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] ext4: use BUG() instead of BUG_ON(1) On Mon, Mar 25, 2019 at 6:00 AM Arnd Bergmann <arnd@...db.de> wrote: > > BUG_ON(1) leads to bogus warnings from clang when > CONFIG_PROFILE_ANNOTATED_BRANCHES is set: > > fs/ext4/inode.c:544:4: error: variable 'retval' is used uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > BUG_ON(1); > ^~~~~~~~~ > include/asm-generic/bug.h:61:36: note: expanded from macro 'BUG_ON' > ^~~~~~~~~~~~~~~~~~~ > include/linux/compiler.h:48:23: note: expanded from macro 'unlikely' > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > fs/ext4/inode.c:591:6: note: uninitialized use occurs here > if (retval > 0 && map->m_flags & EXT4_MAP_MAPPED) { > ^~~~~~ > fs/ext4/inode.c:544:4: note: remove the 'if' if its condition is always true > BUG_ON(1); > ^ > include/asm-generic/bug.h:61:32: note: expanded from macro 'BUG_ON' > ^ > fs/ext4/inode.c:502:12: note: initialize the variable 'retval' to silence this warning > > Change it to BUG() so clang can see that this code path can never > continue. Thanks for the patch; I suspect the definition of `unlikely` is tricky to "see through." This is more concise about what we want to do in these cases anyways. Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com> > > Signed-off-by: Arnd Bergmann <arnd@...db.de> > --- > fs/ext4/extents_status.c | 4 ++-- > fs/ext4/inode.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c > index 2b439afafe13..023a3eb3afa3 100644 > --- a/fs/ext4/extents_status.c > +++ b/fs/ext4/extents_status.c > @@ -711,7 +711,7 @@ static void ext4_es_insert_extent_ind_check(struct inode *inode, > * We don't need to check unwritten extent because > * indirect-based file doesn't have it. > */ > - BUG_ON(1); > + BUG(); > } > } else if (retval == 0) { > if (ext4_es_is_written(es)) { > @@ -780,7 +780,7 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes) > } > p = &(*p)->rb_right; > } else { > - BUG_ON(1); > + BUG(); > return -EINVAL; > } > } > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index b32a57bc5d5d..190f0478582a 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -541,7 +541,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, > map->m_len = retval; > retval = 0; > } else { > - BUG_ON(1); > + BUG(); > } > #ifdef ES_AGGRESSIVE_TEST > ext4_map_blocks_es_recheck(handle, inode, map, > @@ -1876,7 +1876,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, > else if (ext4_es_is_unwritten(&es)) > map->m_flags |= EXT4_MAP_UNWRITTEN; > else > - BUG_ON(1); > + BUG(); > > #ifdef ES_AGGRESSIVE_TEST > ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, 0); > -- > 2.20.0 > -- Thanks, ~Nick Desaulniers
Powered by blists - more mailing lists