| Message ID | 20220121134522.832207-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: initialize variable cancel | expand |
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 >
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;
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.
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
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);
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.
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;