* Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)
2019-03-25 13:00 [PATCH] ext4: use BUG() instead of BUG_ON(1) Arnd Bergmann
@ 2019-03-25 17:29 ` Nick Desaulniers
2019-03-25 17:30 ` Jan Kara
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-03-25 17:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Theodore Ts'o, Andreas Dilger, clang-built-linux,
Nathan Chancellor, Eric Whitney, Sean Fu, Jan Kara, Eric Biggers,
linux-ext4, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)
2019-03-25 13:00 [PATCH] ext4: use BUG() instead of BUG_ON(1) Arnd Bergmann
2019-03-25 17:29 ` Nick Desaulniers
@ 2019-03-25 17:30 ` Jan Kara
2019-03-25 18:14 ` Darrick J. Wong
2019-04-07 16:29 ` Theodore Ts'o
3 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2019-03-25 17:30 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Theodore Ts'o, Andreas Dilger, clang-built-linux,
Nick Desaulniers, Nathan Chancellor, Eric Whitney, Sean Fu,
Jan Kara, Eric Biggers, linux-ext4, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)
2019-03-25 13:00 [PATCH] ext4: use BUG() instead of BUG_ON(1) Arnd Bergmann
2019-03-25 17:29 ` Nick Desaulniers
2019-03-25 17:30 ` Jan Kara
@ 2019-03-25 18:14 ` Darrick J. Wong
2019-03-25 19:59 ` Andreas Dilger
2019-04-07 16:29 ` Theodore Ts'o
3 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-03-25 18:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Theodore Ts'o, Andreas Dilger, clang-built-linux,
Nick Desaulniers, Nathan Chancellor, Eric Whitney, Sean Fu,
Jan Kara, Eric Biggers, linux-ext4, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)
2019-03-25 18:14 ` Darrick J. Wong
@ 2019-03-25 19:59 ` Andreas Dilger
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2019-03-25 19:59 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Arnd Bergmann, Theodore Ts'o, clang-built-linux,
Nick Desaulniers, Nathan Chancellor, Eric Whitney, Sean Fu,
Jan Kara, Eric Biggers, Ext4 Developers List, open list
[-- Attachment #1: Type: text/plain, Size: 4033 bytes --]
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
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: use BUG() instead of BUG_ON(1)
2019-03-25 13:00 [PATCH] ext4: use BUG() instead of BUG_ON(1) Arnd Bergmann
` (2 preceding siblings ...)
2019-03-25 18:14 ` Darrick J. Wong
@ 2019-04-07 16:29 ` Theodore Ts'o
3 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2019-04-07 16:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andreas Dilger, clang-built-linux, Nick Desaulniers,
Nathan Chancellor, Eric Whitney, Sean Fu, Jan Kara, Eric Biggers,
linux-ext4, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread