Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)

[Andreas Dilger]

On Mar 25, 2019, at 12:14 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> 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@xxxxxxxx>
>> ---
>> 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

Attachment: signature.asc
Description: Message signed with OpenPGP