[PATCH] fs/btrfs: Fix uninitialized variable

[PATCH] fs/btrfs: Fix uninitialized variable

[Khaled ROMDHANI]

Fix the warning: variable 'zone' is used
uninitialized whenever '?:' condition is true.

Fix that by preventing the code to reach
the last assertion. If the variable 'mirror'
is invalid, the assertion fails and we return
immediately.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
---
 fs/btrfs/zoned.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 8250ab3f0868..23da9d8dc184 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -145,7 +145,7 @@ static inline u32 sb_zone_number(int shift, int mirror)
 	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
 	default:
 		ASSERT((u32)mirror < 3);
-		break;
+		return 0;
 	}
 
 	ASSERT(zone <= U32_MAX);

base-commit: b5c294aac8a6164ddf38bfbdd1776091b4a1eeba
-- 
2.17.1

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Christophe JAILLET]

Le 02/05/2021 à 00:50, Khaled ROMDHANI a écrit :
> Fix the warning: variable 'zone' is used
> uninitialized whenever '?:' condition is true.
> 
> Fix that by preventing the code to reach
> the last assertion. If the variable 'mirror'
> is invalid, the assertion fails and we return
> immediately.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
> ---
>   fs/btrfs/zoned.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 8250ab3f0868..23da9d8dc184 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -145,7 +145,7 @@ static inline u32 sb_zone_number(int shift, int mirror)
>   	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
>   	default:
>   		ASSERT((u32)mirror < 3);
> -		break;
> +		return 0;
>   	}
>   
>   	ASSERT(zone <= U32_MAX);
> 
> base-commit: b5c294aac8a6164ddf38bfbdd1776091b4a1eeba
> 
Hi,

just a few comments.

If I understand correctly, what you try to do is to silence a compiler 
warning if no case branch is taken.

First, all your proposals are based on the previous one.
I find it hard to follow because we don't easily see what are the 
differences since the beginning.

The "base-commit" at the bottom of your mail, is related to your own 
local tree, I guess. It can't be used by any-one.

My understanding it that a patch, should it be v2, v3..., must apply to 
the current tree. (In my case, it is the latest linux-next)
This is not the case here and you have to apply each step to see the 
final result.

Should this version be fine, a maintainer wouldn't be able to apply it 
as-is.

You also try to take into account previous comments to check for 
incorrect negative values for minor and catch (the can't happen today) 
cases, should BTRFS_SUPER_MIRROR_MAX change and this function remain the 
same.

So, why hard-coding '3'?
The reason of magic numbers are hard to remember. You should avoid them 
or add a comment about it.

My own personal variation would be something like the code below (untested).

Hope this helps.

CJ


diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 70b23a0d03b1..75fe5f001d8b 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -138,11 +138,14 @@ static inline u32 sb_zone_number(int shift, int 
mirror)
  {
  	u64 zone;

-	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
+	ASSERT(mirror >= 0 && mirror < BTRFS_SUPER_MIRROR_MAX);
  	switch (mirror) {
  	case 0: zone = 0; break;
  	case 1: zone = 1ULL << (BTRFS_SB_LOG_FIRST_SHIFT - shift); break;
  	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
+	default:
+		ASSERT(! "mirror < BTRFS_SUPER_MIRROR_MAX but not handled above.");
+		return 0;
  	}

  	ASSERT(zone <= U32_MAX);

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Khaled Romdhani]

On Sun, May 02, 2021 at 12:17:51PM +0200, Christophe JAILLET wrote:
> Le 02/05/2021 à 00:50, Khaled ROMDHANI a écrit :
> > Fix the warning: variable 'zone' is used
> > uninitialized whenever '?:' condition is true.
> > 
> > Fix that by preventing the code to reach
> > the last assertion. If the variable 'mirror'
> > is invalid, the assertion fails and we return
> > immediately.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
> > ---
> >   fs/btrfs/zoned.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> > index 8250ab3f0868..23da9d8dc184 100644
> > --- a/fs/btrfs/zoned.c
> > +++ b/fs/btrfs/zoned.c
> > @@ -145,7 +145,7 @@ static inline u32 sb_zone_number(int shift, int mirror)
> >   	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
> >   	default:
> >   		ASSERT((u32)mirror < 3);
> > -		break;
> > +		return 0;
> >   	}
> >   	ASSERT(zone <= U32_MAX);
> > 
> > base-commit: b5c294aac8a6164ddf38bfbdd1776091b4a1eeba
> > 
> Hi,
> 
> just a few comments.
> 
> If I understand correctly, what you try to do is to silence a compiler
> warning if no case branch is taken.
> 
> First, all your proposals are based on the previous one.
> I find it hard to follow because we don't easily see what are the
> differences since the beginning.
> 
> The "base-commit" at the bottom of your mail, is related to your own local
> tree, I guess. It can't be used by any-one.
> 
> My understanding it that a patch, should it be v2, v3..., must apply to the
> current tree. (In my case, it is the latest linux-next)
> This is not the case here and you have to apply each step to see the final
> result.
> 
> Should this version be fine, a maintainer wouldn't be able to apply it
> as-is.
> 
> You also try to take into account previous comments to check for incorrect
> negative values for minor and catch (the can't happen today) cases, should
> BTRFS_SUPER_MIRROR_MAX change and this function remain the same.
> 
> So, why hard-coding '3'?
> The reason of magic numbers are hard to remember. You should avoid them or
> add a comment about it.
> 
> My own personal variation would be something like the code below (untested).
> 
> Hope this helps.
> 
> CJ
> 
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 70b23a0d03b1..75fe5f001d8b 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -138,11 +138,14 @@ static inline u32 sb_zone_number(int shift, int
> mirror)
>  {
>  	u64 zone;
> 
> -	ASSERT(mirror < BTRFS_SUPER_MIRROR_MAX);
> +	ASSERT(mirror >= 0 && mirror < BTRFS_SUPER_MIRROR_MAX);
>  	switch (mirror) {
>  	case 0: zone = 0; break;
>  	case 1: zone = 1ULL << (BTRFS_SB_LOG_FIRST_SHIFT - shift); break;
>  	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
> +	default:
> +		ASSERT(! "mirror < BTRFS_SUPER_MIRROR_MAX but not handled above.");
> +		return 0;
>  	}
> 
>  	ASSERT(zone <= U32_MAX);
>

Thank you for all of your comments. Yes, of course, they will help me.
I will try to handle that more properly.
Thanks again.

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Dan Carpenter]

On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
> Fix the warning: variable 'zone' is used
> uninitialized whenever '?:' condition is true.
> 
> Fix that by preventing the code to reach
> the last assertion. If the variable 'mirror'
> is invalid, the assertion fails and we return
> immediately.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
> ---

This is not how you send a v4 patch...  v2 patches have to apply to the
original code and not on top of the patched code.

I sort of think you should find a different thing to work on.  This code
works fine as-is.  Just leave it and try to find a real bug and fix that
instead.

regards,
dan carpenter

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Khaled Romdhani]

On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote:
> On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
> > Fix the warning: variable 'zone' is used
> > uninitialized whenever '?:' condition is true.
> > 
> > Fix that by preventing the code to reach
> > the last assertion. If the variable 'mirror'
> > is invalid, the assertion fails and we return
> > immediately.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
> > ---
> 
> This is not how you send a v4 patch...  v2 patches have to apply to the
> original code and not on top of the patched code.
> 
> I sort of think you should find a different thing to work on.  This code
> works fine as-is.  Just leave it and try to find a real bug and fix that
> instead.
> 
> regards,
> dan carpenter
>

Sorry, I was wrong and I shall send a proper V4.

Yes, this code works fine just a warning caught by Coverity scan analysis. 
So the idea behind the patch is to fix the warning. To do that and as suggested by 
David Sterba, it would be rather to add a default case. So we fix the warning 
through the enhancement of the switch statement (some sort of 2*1).

Yes, I will always try to find other bugs. It is a pleasure for me to do that.

Thanks.

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Dan Carpenter]

On Mon, May 03, 2021 at 11:13:12AM +0100, Khaled Romdhani wrote:
> On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote:
> > On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
> > > Fix the warning: variable 'zone' is used
> > > uninitialized whenever '?:' condition is true.
> > > 
> > > Fix that by preventing the code to reach
> > > the last assertion. If the variable 'mirror'
> > > is invalid, the assertion fails and we return
> > > immediately.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
> > > ---
> > 
> > This is not how you send a v4 patch...  v2 patches have to apply to the
> > original code and not on top of the patched code.
> > 
> > I sort of think you should find a different thing to work on.  This code
> > works fine as-is.  Just leave it and try to find a real bug and fix that
> > instead.
> > 
> > regards,
> > dan carpenter
> >
> 
> Sorry, I was wrong and I shall send a proper V4.
> 
> Yes, this code works fine just a warning caught by Coverity scan analysis. 

We're going to a lot of work to silence a static checker false positive.
As a rule, I tell people to ignore the static checker when it is wrong.

Btw, Smatch parses this code correctly and understands that the callers
only pass valid values for "mirror".

regards,
dan carpenter

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[Colin Ian King]

On 03/05/2021 12:55, Dan Carpenter wrote:
> On Mon, May 03, 2021 at 11:13:12AM +0100, Khaled Romdhani wrote:
>> On Mon, May 03, 2021 at 10:23:22AM +0300, Dan Carpenter wrote:
>>> On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
>>>> Fix the warning: variable 'zone' is used
>>>> uninitialized whenever '?:' condition is true.
>>>>
>>>> Fix that by preventing the code to reach
>>>> the last assertion. If the variable 'mirror'
>>>> is invalid, the assertion fails and we return
>>>> immediately.
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>
>>>> ---
>>>
>>> This is not how you send a v4 patch...  v2 patches have to apply to the
>>> original code and not on top of the patched code.
>>>
>>> I sort of think you should find a different thing to work on.  This code
>>> works fine as-is.  Just leave it and try to find a real bug and fix that
>>> instead.
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Sorry, I was wrong and I shall send a proper V4.
>>
>> Yes, this code works fine just a warning caught by Coverity scan analysis. 
> 
> We're going to a lot of work to silence a static checker false positive.
> As a rule, I tell people to ignore the static checker when it is wrong.
> 
> Btw, Smatch parses this code correctly and understands that the callers
> only pass valid values for "mirror".

..and Coverity does report a lot of false positives, so one needs to be
really sure the issue is a genuine issue rather than a warning that can
be ignore.

Colin

> 
> regards,
> dan carpenter
> 

Re: [PATCH] fs/btrfs: Fix uninitialized variable

[David Sterba]

On Sat, May 01, 2021 at 11:50:46PM +0100, Khaled ROMDHANI wrote:
> Fix the warning: variable 'zone' is used
> uninitialized whenever '?:' condition is true.
> 
> Fix that by preventing the code to reach
> the last assertion. If the variable 'mirror'
> is invalid, the assertion fails and we return
> immediately.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Khaled ROMDHANI <khaledromdhani216@gmail.com>

This took several rounds and none of them was close to what I'd consider
a proper fix, for something that's not really important. As Dan said,
smatch does understand the values passed from the callers and the
function is a static inline so the complete information is available. No
tricky analysis is required, so why does not coverity see that too?

We use assertions to namely catch programmer errors and API misuse,
anything that can happen at runtime or depends on input needs proper
checks and error handling. But for the super block copies, the constant
won't change so all we want is to catch the stupid errors.

> ---
>  fs/btrfs/zoned.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 8250ab3f0868..23da9d8dc184 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -145,7 +145,7 @@ static inline u32 sb_zone_number(int shift, int mirror)
>  	case 2: zone = 1ULL << (BTRFS_SB_LOG_SECOND_SHIFT - shift); break;
>  	default:
>  		ASSERT((u32)mirror < 3);
> -		break;
> +		return 0;

It's been pointed out that this does not apply on the current code but
on top of previous versions, so it's not making it easy for me to apply
the patch and do maybe some tweaks only.

I don't mind merging trivial patches, people can learn the process and
few iterations are not a big deal. What I also hope for is to get some
understanding of the code being changed and not just silencing some
tools' warnings.