Re: [kdave-btrfs-devel:for-next-20220307 108/147] fs/btrfs/scrub.c:3146 scrub_raid56_data_stripe_for_parity() error: we previously assumed 'bioc' could be null (see line 3138)

From: Qu Wenruo
Date: Tue Mar 08 2022 - 08:56:17 EST

Next message: Andrew Lunn: "Re: [PATCH] net: arc_emac: Fix use after free in arc_mdio_probe()"
Previous message: Qian Cai: "Re: [PATCH] configs/debug: set CONFIG_DEBUG_INFO=y properly"
In reply to: Dan Carpenter: "Re: [kdave-btrfs-devel:for-next-20220307 108/147] fs/btrfs/scrub.c:3146 scrub_raid56_data_stripe_for_parity() error: we previously assumed 'bioc' could be null (see line 3138)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2022/3/8 21:42, Dan Carpenter wrote:

On Tue, Mar 08, 2022 at 04:09:54PM +0300, Dan Carpenter wrote:

tree: https://github.com/kdave/btrfs-devel.git for-next-20220307
head: 912dedd70aeb485247c507115704ea7d137d758b
commit: 80cd926eefca522182ee3cf04d8e9984073d34d1 [108/147] btrfs: refactor scrub_raid56_parity()
config: i386-randconfig-m021-20220307 (https://download.01.org/0day-ci/archive/20220308/202203081837.zOttrQRN-lkp@xxxxxxxxx/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

New smatch warnings:
fs/btrfs/scrub.c:3171 scrub_raid56_data_stripe_for_parity() error: uninitialized symbol 'extent_start'.
fs/btrfs/scrub.c:3172 scrub_raid56_data_stripe_for_parity() error: uninitialized symbol 'extent_size'.

Old smatch warnings:
fs/btrfs/scrub.c:3419 scrub_simple_mirror() error: uninitialized symbol 'ret'.

vim +/bioc +3146 fs/btrfs/scrub.c

093741d4cda2cb4 Qu Wenruo 2022-02-18 3021
80cd926eefca522 Qu Wenruo 2022-02-18 3022 static int scrub_raid56_data_stripe_for_parity(struct scrub_ctx *sctx,
80cd926eefca522 Qu Wenruo 2022-02-18 3023 struct scrub_parity *sparity,
5a6ac9eacb49143 Miao Xie 2014-11-06 3024 struct map_lookup *map,
5a6ac9eacb49143 Miao Xie 2014-11-06 3025 struct btrfs_device *sdev,
80cd926eefca522 Qu Wenruo 2022-02-18 3026 struct btrfs_path *path,
80cd926eefca522 Qu Wenruo 2022-02-18 3027 u64 logical)
5a6ac9eacb49143 Miao Xie 2014-11-06 3028 {
fb456252d3d9c05 Jeff Mahoney 2016-06-22 3029 struct btrfs_fs_info *fs_info = sctx->fs_info;
80cd926eefca522 Qu Wenruo 2022-02-18 3030 struct btrfs_root *extent_root = btrfs_extent_root(fs_info, logical);
80cd926eefca522 Qu Wenruo 2022-02-18 3031 struct btrfs_root *csum_root = btrfs_csum_root(fs_info, logical);
5a6ac9eacb49143 Miao Xie 2014-11-06 3032 struct btrfs_key key;
80cd926eefca522 Qu Wenruo 2022-02-18 3033 u64 extent_start;
80cd926eefca522 Qu Wenruo 2022-02-18 3034 u64 extent_size;
80cd926eefca522 Qu Wenruo 2022-02-18 3035 int ret;
2522dbe86b54ff0 Qu Wenruo 2021-12-14 3036
80cd926eefca522 Qu Wenruo 2022-02-18 3037 ASSERT(map->type & BTRFS_BLOCK_GROUP_RAID56_MASK);
5a6ac9eacb49143 Miao Xie 2014-11-06 3038
80cd926eefca522 Qu Wenruo 2022-02-18 3039 /* Path should not be populated */
80cd926eefca522 Qu Wenruo 2022-02-18 3040 ASSERT(!path->nodes[0]);
5a6ac9eacb49143 Miao Xie 2014-11-06 3041
5a6ac9eacb49143 Miao Xie 2014-11-06 3042 if (btrfs_fs_incompat(fs_info, SKINNY_METADATA))
5a6ac9eacb49143 Miao Xie 2014-11-06 3043 key.type = BTRFS_METADATA_ITEM_KEY;
5a6ac9eacb49143 Miao Xie 2014-11-06 3044 else
5a6ac9eacb49143 Miao Xie 2014-11-06 3045 key.type = BTRFS_EXTENT_ITEM_KEY;
80cd926eefca522 Qu Wenruo 2022-02-18 3046 key.objectid = logical;
5a6ac9eacb49143 Miao Xie 2014-11-06 3047 key.offset = (u64)-1;
5a6ac9eacb49143 Miao Xie 2014-11-06 3048
80cd926eefca522 Qu Wenruo 2022-02-18 3049 ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
5a6ac9eacb49143 Miao Xie 2014-11-06 3050 if (ret < 0)
80cd926eefca522 Qu Wenruo 2022-02-18 3051 return ret;
5a6ac9eacb49143 Miao Xie 2014-11-06 3052
5a6ac9eacb49143 Miao Xie 2014-11-06 3053 if (ret > 0) {
80cd926eefca522 Qu Wenruo 2022-02-18 3054 ret = btrfs_previous_extent_item(extent_root, path, 0);
5a6ac9eacb49143 Miao Xie 2014-11-06 3055 if (ret < 0)
80cd926eefca522 Qu Wenruo 2022-02-18 3056 return ret;
5a6ac9eacb49143 Miao Xie 2014-11-06 3057 if (ret > 0) {
5a6ac9eacb49143 Miao Xie 2014-11-06 3058 btrfs_release_path(path);
80cd926eefca522 Qu Wenruo 2022-02-18 3059 ret = btrfs_search_slot(NULL, extent_root, &key, path,
80cd926eefca522 Qu Wenruo 2022-02-18 3060 0, 0);
5a6ac9eacb49143 Miao Xie 2014-11-06 3061 if (ret < 0)
80cd926eefca522 Qu Wenruo 2022-02-18 3062 return ret;
5a6ac9eacb49143 Miao Xie 2014-11-06 3063 }
5a6ac9eacb49143 Miao Xie 2014-11-06 3064 }
5a6ac9eacb49143 Miao Xie 2014-11-06 3065
5a6ac9eacb49143 Miao Xie 2014-11-06 3066 while (1) {
80cd926eefca522 Qu Wenruo 2022-02-18 3067 struct btrfs_io_context *bioc = NULL;
80cd926eefca522 Qu Wenruo 2022-02-18 3068 struct btrfs_device *extent_dev;
80cd926eefca522 Qu Wenruo 2022-02-18 3069 struct btrfs_extent_item *ei;
80cd926eefca522 Qu Wenruo 2022-02-18 3070 struct extent_buffer *l;
80cd926eefca522 Qu Wenruo 2022-02-18 3071 int slot;
80cd926eefca522 Qu Wenruo 2022-02-18 3072 u64 mapped_length;
80cd926eefca522 Qu Wenruo 2022-02-18 3073 u64 extent_flags;
80cd926eefca522 Qu Wenruo 2022-02-18 3074 u64 extent_gen;
80cd926eefca522 Qu Wenruo 2022-02-18 3075 u64 extent_physical;
80cd926eefca522 Qu Wenruo 2022-02-18 3076 u64 extent_mirror_num;
5a6ac9eacb49143 Miao Xie 2014-11-06 3077
5a6ac9eacb49143 Miao Xie 2014-11-06 3078 l = path->nodes[0];
5a6ac9eacb49143 Miao Xie 2014-11-06 3079 slot = path->slots[0];
5a6ac9eacb49143 Miao Xie 2014-11-06 3080 if (slot >= btrfs_header_nritems(l)) {
80cd926eefca522 Qu Wenruo 2022-02-18 3081 ret = btrfs_next_leaf(extent_root, path);
5a6ac9eacb49143 Miao Xie 2014-11-06 3082 if (ret == 0)
5a6ac9eacb49143 Miao Xie 2014-11-06 3083 continue;
5a6ac9eacb49143 Miao Xie 2014-11-06 3084
80cd926eefca522 Qu Wenruo 2022-02-18 3085 /* No more extent items or error, exit */
5a6ac9eacb49143 Miao Xie 2014-11-06 3086 break;

Oops. I misread what Smatch was doing. It's this break that it's
complaining about. "ret" is negative and "extent_start" is uninitialized
on the first iteration through the loop.

The goto would not trigger the warning.

Oh, then you're completely correct, and this is even affecting the current code.

The fix is not that complex, instead of break, we should release path and return (or add a new out label).

As all (ret < 0) break should not go through the scrub_parity_mark_sectors_error() call, until we initialized @extent_start/@extent_size.

@David, what's the proper way to fix it?

Just send a patch to fix the latest code so you can fold it into "btrfs: use find_first_extent_item() to replace the open-coded extent item search"

Or I should submit fix for both "btrfs: use find_first_extent_item() to replace the open-coded extent item search" and "btrfs: refactor scrub_raid56_parity()"?

Thanks,
Qu

5a6ac9eacb49143 Miao Xie 2014-11-06 3087 }
5a6ac9eacb49143 Miao Xie 2014-11-06 3088 btrfs_item_key_to_cpu(l, &key, slot);
5a6ac9eacb49143 Miao Xie 2014-11-06 3089
d7cad2389560f32 Zhao Lei 2015-07-22 3090 if (key.type != BTRFS_EXTENT_ITEM_KEY &&
d7cad2389560f32 Zhao Lei 2015-07-22 3091 key.type != BTRFS_METADATA_ITEM_KEY)
d7cad2389560f32 Zhao Lei 2015-07-22 3092 goto next;
d7cad2389560f32 Zhao Lei 2015-07-22 3093
5a6ac9eacb49143 Miao Xie 2014-11-06 3094 if (key.type == BTRFS_METADATA_ITEM_KEY)
80cd926eefca522 Qu Wenruo 2022-02-18 3095 extent_size = fs_info->nodesize;
5a6ac9eacb49143 Miao Xie 2014-11-06 3096 else
80cd926eefca522 Qu Wenruo 2022-02-18 3097 extent_size = key.offset;
5a6ac9eacb49143 Miao Xie 2014-11-06 3098
80cd926eefca522 Qu Wenruo 2022-02-18 3099 if (key.objectid + extent_size <= logical)
5a6ac9eacb49143 Miao Xie 2014-11-06 3100 goto next;
5a6ac9eacb49143 Miao Xie 2014-11-06 3101
80cd926eefca522 Qu Wenruo 2022-02-18 3102 /* Beyond this data stripe */
80cd926eefca522 Qu Wenruo 2022-02-18 3103 if (key.objectid >= logical + map->stripe_len)
5a6ac9eacb49143 Miao Xie 2014-11-06 3104 break;

On this break "ret" is zero so it wouldn't trigger an error.

5a6ac9eacb49143 Miao Xie 2014-11-06 3105
80cd926eefca522 Qu Wenruo 2022-02-18 3106 ei = btrfs_item_ptr(l, slot, struct btrfs_extent_item);
80cd926eefca522 Qu Wenruo 2022-02-18 3107 extent_flags = btrfs_extent_flags(l, ei);
80cd926eefca522 Qu Wenruo 2022-02-18 3108 extent_gen = btrfs_extent_generation(l, ei);
5a6ac9eacb49143 Miao Xie 2014-11-06 3109
80cd926eefca522 Qu Wenruo 2022-02-18 3110 if ((extent_flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) &&
80cd926eefca522 Qu Wenruo 2022-02-18 3111 (key.objectid < logical || key.objectid + extent_size >
80cd926eefca522 Qu Wenruo 2022-02-18 3112 logical + map->stripe_len)) {
5d163e0e68ce743 Jeff Mahoney 2016-09-20 3113 btrfs_err(fs_info,
5d163e0e68ce743 Jeff Mahoney 2016-09-20 3114 "scrub: tree block %llu spanning stripes, ignored. logical=%llu",
80cd926eefca522 Qu Wenruo 2022-02-18 3115 key.objectid, logical);
9799d2c32bef6fb Zhao Lei 2015-08-25 3116 spin_lock(&sctx->stat_lock);
9799d2c32bef6fb Zhao Lei 2015-08-25 3117 sctx->stat.uncorrectable_errors++;
9799d2c32bef6fb Zhao Lei 2015-08-25 3118 spin_unlock(&sctx->stat_lock);
5a6ac9eacb49143 Miao Xie 2014-11-06 3119 goto next;

This goto next is what triggers the uninitialized variable warnings for
extent_start and extent_size.

No. This is fine.

5a6ac9eacb49143 Miao Xie 2014-11-06 3120 }
5a6ac9eacb49143 Miao Xie 2014-11-06 3121
80cd926eefca522 Qu Wenruo 2022-02-18 3122 extent_start = key.objectid;

And anything after this is fine.

regards,
dan carpenter

80cd926eefca522 Qu Wenruo 2022-02-18 3123 ASSERT(extent_size <= U32_MAX);
5a6ac9eacb49143 Miao Xie 2014-11-06 3124
80cd926eefca522 Qu Wenruo 2022-02-18 3125 /* Truncate the range inside the data stripe */
80cd926eefca522 Qu Wenruo 2022-02-18 3126 if (extent_start < logical) {
80cd926eefca522 Qu Wenruo 2022-02-18 3127 extent_size -= logical - extent_start;
80cd926eefca522 Qu Wenruo 2022-02-18 3128 extent_start = logical;
80cd926eefca522 Qu Wenruo 2022-02-18 3129 }
80cd926eefca522 Qu Wenruo 2022-02-18 3130 if (extent_start + extent_size > logical + map->stripe_len)
80cd926eefca522 Qu Wenruo 2022-02-18 3131 extent_size = logical + map->stripe_len - extent_start;
5a6ac9eacb49143 Miao Xie 2014-11-06 3132
80cd926eefca522 Qu Wenruo 2022-02-18 3133 scrub_parity_mark_sectors_data(sparity, extent_start, extent_size);
5a6ac9eacb49143 Miao Xie 2014-11-06 3134
80cd926eefca522 Qu Wenruo 2022-02-18 3135 mapped_length = extent_size;
80cd926eefca522 Qu Wenruo 2022-02-18 3136 ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, extent_start,
80cd926eefca522 Qu Wenruo 2022-02-18 3137 &mapped_length, &bioc, 0);
80cd926eefca522 Qu Wenruo 2022-02-18 3138 if (!ret && (!bioc || mapped_length < extent_size))
4a770891d9ddf94 Omar Sandoval 2015-06-19 3139 ret = -EIO;
4a770891d9ddf94 Omar Sandoval 2015-06-19 3140 if (ret) {
4c6646117912397 Qu Wenruo 2021-09-15 3141 btrfs_put_bioc(bioc);
80cd926eefca522 Qu Wenruo 2022-02-18 3142 scrub_parity_mark_sectors_error(sparity, extent_start,
80cd926eefca522 Qu Wenruo 2022-02-18 3143 extent_size);
80cd926eefca522 Qu Wenruo 2022-02-18 3144 break;
4a770891d9ddf94 Omar Sandoval 2015-06-19 3145 }
4c6646117912397 Qu Wenruo 2021-09-15 3146 extent_physical = bioc->stripes[0].physical;
4c6646117912397 Qu Wenruo 2021-09-15 3147 extent_mirror_num = bioc->mirror_num;
4c6646117912397 Qu Wenruo 2021-09-15 3148 extent_dev = bioc->stripes[0].dev;
4c6646117912397 Qu Wenruo 2021-09-15 3149 btrfs_put_bioc(bioc);
5a6ac9eacb49143 Miao Xie 2014-11-06 3150
80cd926eefca522 Qu Wenruo 2022-02-18 3151 ret = btrfs_lookup_csums_range(csum_root, extent_start,
80cd926eefca522 Qu Wenruo 2022-02-18 3152 extent_start + extent_size - 1,
5a6ac9eacb49143 Miao Xie 2014-11-06 3153 &sctx->csum_list, 1);
5a6ac9eacb49143 Miao Xie 2014-11-06 3154 if (ret)
80cd926eefca522 Qu Wenruo 2022-02-18 3155 break;
6fa96d72f79a155 Zhao Lei 2015-07-21 3156
80cd926eefca522 Qu Wenruo 2022-02-18 3157 ret = scrub_extent_for_parity(sparity, extent_start,
80cd926eefca522 Qu Wenruo 2022-02-18 3158 extent_size, extent_physical,
80cd926eefca522 Qu Wenruo 2022-02-18 3159 extent_dev, extent_flags,
80cd926eefca522 Qu Wenruo 2022-02-18 3160 extent_gen, extent_mirror_num);
6fa96d72f79a155 Zhao Lei 2015-07-21 3161 scrub_free_csums(sctx);
6fa96d72f79a155 Zhao Lei 2015-07-21 3162
5a6ac9eacb49143 Miao Xie 2014-11-06 3163 if (ret)
5a6ac9eacb49143 Miao Xie 2014-11-06 3164 break;
5a6ac9eacb49143 Miao Xie 2014-11-06 3165
5a6ac9eacb49143 Miao Xie 2014-11-06 3166 cond_resched();
5a6ac9eacb49143 Miao Xie 2014-11-06 3167 next:
5a6ac9eacb49143 Miao Xie 2014-11-06 3168 path->slots[0]++;
5a6ac9eacb49143 Miao Xie 2014-11-06 3169 }
80cd926eefca522 Qu Wenruo 2022-02-18 3170 if (ret < 0)
80cd926eefca522 Qu Wenruo 2022-02-18 @3171 scrub_parity_mark_sectors_error(sparity, extent_start,
80cd926eefca522 Qu Wenruo 2022-02-18 @3172 extent_size);
5a6ac9eacb49143 Miao Xie 2014-11-06 3173 btrfs_release_path(path);
80cd926eefca522 Qu Wenruo 2022-02-18 3174 return ret;
80cd926eefca522 Qu Wenruo 2022-02-18 3175 }
5a6ac9eacb49143 Miao Xie 2014-11-06 3176

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx

Next message: Andrew Lunn: "Re: [PATCH] net: arc_emac: Fix use after free in arc_mdio_probe()"
Previous message: Qian Cai: "Re: [PATCH] configs/debug: set CONFIG_DEBUG_INFO=y properly"
In reply to: Dan Carpenter: "Re: [kdave-btrfs-devel:for-next-20220307 108/147] fs/btrfs/scrub.c:3146 scrub_raid56_data_stripe_for_parity() error: we previously assumed 'bioc' could be null (see line 3138)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]