[PATCH] btrfs: initialize variable cancel

[PATCH] btrfs: initialize variable cancel

[trix]

From: Tom Rix <trix@redhat.com>

Clang static analysis reports this problem
ioctl.c:3333:8: warning: 3rd function call argument is an
  uninitialized value
    ret = exclop_start_or_cancel_reloc(fs_info,

cancel is only set in one branch of an if-check and is
always used.  So initialize to false.

Fixes: 1a15eb724aae ("btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 fs/btrfs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 190ad8af4f45a..26e82379747f8 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3308,7 +3308,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	struct block_device *bdev = NULL;
 	fmode_t mode;
 	int ret;
-	bool cancel;
+	bool cancel = false;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-- 
2.26.3

Re: [PATCH] btrfs: initialize variable cancel

[Filipe Manana]

On Fri, Jan 21, 2022 at 05:45:22AM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this problem
> ioctl.c:3333:8: warning: 3rd function call argument is an
>   uninitialized value
>     ret = exclop_start_or_cancel_reloc(fs_info,
> 
> cancel is only set in one branch of an if-check and is
> always used.  So initialize to false.
> 
> Fixes: 1a15eb724aae ("btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls")
> Signed-off-by: Tom Rix <trix@redhat.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Could use a more precise subject like for example:

  "btrfs: fix use of uninitialized variable at rm device ioctl"

Anyway, it looks good.
Thanks.

> ---
>  fs/btrfs/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 190ad8af4f45a..26e82379747f8 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3308,7 +3308,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>  	struct block_device *bdev = NULL;
>  	fmode_t mode;
>  	int ret;
> -	bool cancel;
> +	bool cancel = false;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> -- 
> 2.26.3
> 

Re: [PATCH] btrfs: initialize variable cancel

[Anand Jain]

On 21/01/2022 21:45, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this problem
> ioctl.c:3333:8: warning: 3rd function call argument is an
>    uninitialized value
>      ret = exclop_start_or_cancel_reloc(fs_info,
> 
> cancel is only set in one branch of an if-check and is
> always used.  So initialize to false.
> 
> Fixes: 1a15eb724aae ("btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>   fs/btrfs/ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 190ad8af4f45a..26e82379747f8 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3308,7 +3308,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>   	struct block_device *bdev = NULL;
>   	fmode_t mode;
>   	int ret;
> -	bool cancel;
> +	bool cancel = false;


  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks,
Anand

>   
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;

Re: [PATCH] btrfs: initialize variable cancel

[David Sterba]

On Fri, Jan 21, 2022 at 05:45:22AM -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this problem
> ioctl.c:3333:8: warning: 3rd function call argument is an
>   uninitialized value
>     ret = exclop_start_or_cancel_reloc(fs_info,
> 
> cancel is only set in one branch of an if-check and is
> always used.  So initialize to false.
> 
> Fixes: 1a15eb724aae ("btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls")
> Signed-off-by: Tom Rix <trix@redhat.com>

Added to misc-next, with the updted subject line, thanks.

Re: [PATCH] btrfs: initialize variable cancel

[Wang Yugui]

Hi,

Mabye we should enable '-Wmaybe-uninitialized' for btrfs 'make W=1'.

There is another warning  reproted by '-Wmaybe-uninitialized' 
in btrfs misc-next.

fs/btrfs/zoned.c:273:28: warning: ‘zno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   memcpy(zinfo->zone_cache + zno, zones,
                            ^

diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
index cec88a6..f1052ce 100644
--- a/fs/btrfs/Makefile
+++ b/fs/btrfs/Makefile
@@ -2,6 +2,7 @@
 
 # Subset of W=1 warnings
 subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
+subdir-ccflags-y += -Wmaybe-uninitialized
 subdir-ccflags-y += -Wmissing-declarations
 subdir-ccflags-y += -Wmissing-format-attribute
 subdir-ccflags-y += -Wmissing-prototypes

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/01/25

> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this problem
> ioctl.c:3333:8: warning: 3rd function call argument is an
>   uninitialized value
>     ret = exclop_start_or_cancel_reloc(fs_info,
> 
> cancel is only set in one branch of an if-check and is
> always used.  So initialize to false.
> 
> Fixes: 1a15eb724aae ("btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  fs/btrfs/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 190ad8af4f45a..26e82379747f8 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3308,7 +3308,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>  	struct block_device *bdev = NULL;
>  	fmode_t mode;
>  	int ret;
> -	bool cancel;
> +	bool cancel = false;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> -- 
> 2.26.3

Re: [PATCH] btrfs: initialize variable cancel

[Johannes Thumshirn]

On Tue, 2022-01-25 at 13:04 +0800, Wang Yugui wrote:
> Hi,
> 
> Mabye we should enable '-Wmaybe-uninitialized' for btrfs 'make W=1'.
> 
> There is another warning  reproted by '-Wmaybe-uninitialized' 
> in btrfs misc-next.
> 
> fs/btrfs/zoned.c:273:28: warning: ‘zno’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>    memcpy(zinfo->zone_cache + zno, zones,
>                             ^


Isn't that one a false positive?

      u32 zno;                                                        

[...]                                                                 
        /* Check cache */                                             
        if (zinfo->zone_cache) {                                      
                unsigned int i;                                       
                                                                               
                ASSERT(IS_ALIGNED(pos, zinfo->zone_size));            
                zno = pos >> zinfo->zone_size_shift;                  
[...]
        }                                                             
                                                                               
[...]    
                                                                      
        /* Populate cache */                                          
        if (zinfo->zone_cache)                                        
                memcpy(zinfo->zone_cache + zno, zones,                
                       sizeof(*zinfo->zone_cache) * *nr_zones);       
                                    

Re: [PATCH] btrfs: initialize variable cancel

[David Sterba]

On Tue, Jan 25, 2022 at 08:30:34AM +0000, Johannes Thumshirn wrote:
> On Tue, 2022-01-25 at 13:04 +0800, Wang Yugui wrote:
> > Hi,
> > 
> > Mabye we should enable '-Wmaybe-uninitialized' for btrfs 'make W=1'.
> > 
> > There is another warning  reproted by '-Wmaybe-uninitialized' 
> > in btrfs misc-next.
> > 
> > fs/btrfs/zoned.c:273:28: warning: ‘zno’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> >    memcpy(zinfo->zone_cache + zno, zones,
> >                             ^
> 
> 
> Isn't that one a false positive?
> 
>       u32 zno;                                                        
> 
> [...]                                                                 
>         /* Check cache */                                             
>         if (zinfo->zone_cache) {                                      
>                 unsigned int i;                                       
>                                                                                
>                 ASSERT(IS_ALIGNED(pos, zinfo->zone_size));            
>                 zno = pos >> zinfo->zone_size_shift;                  
> [...]
>         }                                                             
>                                                                                
> [...]    
>                                                                       
>         /* Populate cache */                                          
>         if (zinfo->zone_cache)                                        
>                 memcpy(zinfo->zone_cache + zno, zones,                
>                        sizeof(*zinfo->zone_cache) * *nr_zones);       
>                                     

Yeah looks like a false positive, maybe it can't reason that
zinfo->zone_cache won't change between the conditions from NULL to valid
pointer.

And as there are other warnings in sb_zone_number we can't set
-Wmaybe-uninitialized by default. I think this could be even more
problematic with older compiler versions.