| Message ID | 20190325130040.1437445-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 1e83bc8156028a938845a869a3317c3cab9630ac |
| Headers | show |
| Series | ext4: use BUG() instead of BUG_ON(1) | expand |
On Mon, Mar 25, 2019 at 6:00 AM Arnd Bergmann <arnd@arndb.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@google.com> > > Signed-off-by: Arnd Bergmann <arnd@arndb.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
On Mon 25-03-19 14:00:25, Arnd Bergmann 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. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Sure. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > 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 > -- Jan Kara <jack@suse.com> SUSE Labs, CR
On Mon, Mar 25, 2019 at 02:00:25PM +0100, Arnd Bergmann 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. I grok that most of these look like "should never get here" assertions, but shouldn't we be converting these BUG*() calls to "shut down fs, bounce error back to userspace" instead of killing the whole kernel? (He says knowing that ripping all of those out is its own project... :P) --D > Signed-off-by: Arnd Bergmann <arnd@arndb.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 >
On Mar 25, 2019, at 12:14 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Mon, Mar 25, 2019 at 02:00:25PM +0100, Arnd Bergmann 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. > > I grok that most of these look like "should never get here" assertions, > but shouldn't we be converting these BUG*() calls to "shut down fs, > bounce error back to userspace" instead of killing the whole kernel? > > (He says knowing that ripping all of those out is its own project... :P) We definitely shouldn't be adding "BUG()" or "BUG_ON()" to active code, but rather call ext4_error() and let the admin to set "errors=panic" if they want this action, or leave "errors=remount-ro" (which is the default these days) and just return an error to userspace. Looking at the nearby code, it is using pr_warn("ES insert assertion failed..." instead of using an actual assertion for the failure cases. I guess a saving grace is that this code is only enabled if ES_AGGRESSIVE_TEST is defined, which it is not by default, so probably it is only when some developer is testing this code. So using BUG() is probably OK in this case. Cheers, Andreas > >> Signed-off-by: Arnd Bergmann <arnd@arndb.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 Cheers, Andreas
On Mon, Mar 25, 2019 at 02:00:25PM +0100, Arnd Bergmann wrote: > BUG_ON(1) leads to bogus warnings from clang when > CONFIG_PROFILE_ANNOTATED_BRANCHES is set: > > .... > > Change it to BUG() so clang can see that this code path can never > continue. Thanks, applied. - Ted
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);
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. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/ext4/extents_status.c | 4 ++-- fs/ext4/inode.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.20.0