| Message ID | 20221014122032.47784-1-jinpu.wang@ionos.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | md/bitmap: Fix bitmap chunk size overflow issues. | expand |
Dear Jack, dear Florian-Ewald, Thank you for the patch. Am 14.10.22 um 14:20 schrieb Jack Wang: > From: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > > - limit bitmap chunk size internal u64 variable to values not overflowing > the u32 bitmap superblock structure variable stored on persistent media. > - assign bitmap chunk size internal u64 variable from unsigned values to > avoid possible sign extension artifacts when assigning from a s32 value. > > The bug has been there since at least kernel 4.0. Did you find this during code review or hit actual problems? If so, a reproducer would be nice to have. (A small nit, should you resend, if you removed the dot/period from the end of the commit message summary/title, that’d be great.) Kind regards, Paul > Cc: stable@vger.kernel.org > > Signed-off-by: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/md/md-bitmap.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c > index bf6dffadbe6f..b266711485a8 100644 > --- a/drivers/md/md-bitmap.c > +++ b/drivers/md/md-bitmap.c > @@ -486,7 +486,7 @@ void md_bitmap_print_sb(struct bitmap *bitmap) > sb = kmap_atomic(bitmap->storage.sb_page); > pr_debug("%s: bitmap file superblock:\n", bmname(bitmap)); > pr_debug(" magic: %08x\n", le32_to_cpu(sb->magic)); > - pr_debug(" version: %d\n", le32_to_cpu(sb->version)); > + pr_debug(" version: %u\n", le32_to_cpu(sb->version)); > pr_debug(" uuid: %08x.%08x.%08x.%08x\n", > le32_to_cpu(*(__le32 *)(sb->uuid+0)), > le32_to_cpu(*(__le32 *)(sb->uuid+4)), > @@ -497,11 +497,11 @@ void md_bitmap_print_sb(struct bitmap *bitmap) > pr_debug("events cleared: %llu\n", > (unsigned long long) le64_to_cpu(sb->events_cleared)); > pr_debug(" state: %08x\n", le32_to_cpu(sb->state)); > - pr_debug(" chunksize: %d B\n", le32_to_cpu(sb->chunksize)); > - pr_debug(" daemon sleep: %ds\n", le32_to_cpu(sb->daemon_sleep)); > + pr_debug(" chunksize: %u B\n", le32_to_cpu(sb->chunksize)); > + pr_debug(" daemon sleep: %us\n", le32_to_cpu(sb->daemon_sleep)); > pr_debug(" sync size: %llu KB\n", > (unsigned long long)le64_to_cpu(sb->sync_size)/2); > - pr_debug("max write behind: %d\n", le32_to_cpu(sb->write_behind)); > + pr_debug("max write behind: %u\n", le32_to_cpu(sb->write_behind)); > kunmap_atomic(sb); > } > > @@ -2105,7 +2105,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > bytes = DIV_ROUND_UP(chunks, 8); > if (!bitmap->mddev->bitmap_info.external) > bytes += sizeof(bitmap_super_t); > - } while (bytes > (space << 9)); > + } while (bytes > (space << 9) && (chunkshift + BITMAP_BLOCK_SHIFT) < > + (BITS_PER_BYTE * sizeof(((bitmap_super_t *)0)->chunksize) - 1)); > } else > chunkshift = ffz(~chunksize) - BITMAP_BLOCK_SHIFT; > > @@ -2150,7 +2151,7 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > bitmap->counts.missing_pages = pages; > bitmap->counts.chunkshift = chunkshift; > bitmap->counts.chunks = chunks; > - bitmap->mddev->bitmap_info.chunksize = 1 << (chunkshift + > + bitmap->mddev->bitmap_info.chunksize = 1UL << (chunkshift + > BITMAP_BLOCK_SHIFT); > > blocks = min(old_counts.chunks << old_counts.chunkshift, > @@ -2176,8 +2177,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > bitmap->counts.missing_pages = old_counts.pages; > bitmap->counts.chunkshift = old_counts.chunkshift; > bitmap->counts.chunks = old_counts.chunks; > - bitmap->mddev->bitmap_info.chunksize = 1 << (old_counts.chunkshift + > - BITMAP_BLOCK_SHIFT); > + bitmap->mddev->bitmap_info.chunksize = > + 1UL << (old_counts.chunkshift + BITMAP_BLOCK_SHIFT); > blocks = old_counts.chunks << old_counts.chunkshift; > pr_warn("Could not pre-allocate in-memory bitmap for cluster raid\n"); > break; > @@ -2534,6 +2535,9 @@ chunksize_store(struct mddev *mddev, const char *buf, size_t len) > if (csize < 512 || > !is_power_of_2(csize)) > return -EINVAL; > + if (csize >= (1UL << (BITS_PER_BYTE * > + sizeof(((bitmap_super_t *)0)->chunksize)))) > + return -EOVERFLOW; > mddev->bitmap_info.chunksize = csize; > return len; > }
On Fri, Oct 14, 2022 at 3:23 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Jack, dear Florian-Ewald, > > > Thank you for the patch. > > Am 14.10.22 um 14:20 schrieb Jack Wang: > > From: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > > > > - limit bitmap chunk size internal u64 variable to values not overflowing > > the u32 bitmap superblock structure variable stored on persistent media. > > - assign bitmap chunk size internal u64 variable from unsigned values to > > avoid possible sign extension artifacts when assigning from a s32 value. > > > > The bug has been there since at least kernel 4.0. > > Did you find this during code review or hit actual problems? If so, a > reproducer would be nice to have. (A small nit, should you resend, if > you removed the dot/period from the end of the commit message > summary/title, that’d be great.) Yes, we can reproduce it. steps: 1: mdadm -C /dev/mdx -l 1 --bitmap=internal --bitmap-chunk=256M -e 1.2 -n2 /dev/rnbd1 /dev/rnbd2 2 resize member device rnbd1 and rnbd2 to 8 TB 3 mdadm --grow /dev/mdx --size=max the bitmap_chunk will overflow. Sure we can improve the commit message. > > Kind regards, > > Paul Thanks for checking! > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Florian-Ewald Mueller <florian-ewald.mueller@ionos.com> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > --- > > drivers/md/md-bitmap.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c > > index bf6dffadbe6f..b266711485a8 100644 > > --- a/drivers/md/md-bitmap.c > > +++ b/drivers/md/md-bitmap.c > > @@ -486,7 +486,7 @@ void md_bitmap_print_sb(struct bitmap *bitmap) > > sb = kmap_atomic(bitmap->storage.sb_page); > > pr_debug("%s: bitmap file superblock:\n", bmname(bitmap)); > > pr_debug(" magic: %08x\n", le32_to_cpu(sb->magic)); > > - pr_debug(" version: %d\n", le32_to_cpu(sb->version)); > > + pr_debug(" version: %u\n", le32_to_cpu(sb->version)); > > pr_debug(" uuid: %08x.%08x.%08x.%08x\n", > > le32_to_cpu(*(__le32 *)(sb->uuid+0)), > > le32_to_cpu(*(__le32 *)(sb->uuid+4)), > > @@ -497,11 +497,11 @@ void md_bitmap_print_sb(struct bitmap *bitmap) > > pr_debug("events cleared: %llu\n", > > (unsigned long long) le64_to_cpu(sb->events_cleared)); > > pr_debug(" state: %08x\n", le32_to_cpu(sb->state)); > > - pr_debug(" chunksize: %d B\n", le32_to_cpu(sb->chunksize)); > > - pr_debug(" daemon sleep: %ds\n", le32_to_cpu(sb->daemon_sleep)); > > + pr_debug(" chunksize: %u B\n", le32_to_cpu(sb->chunksize)); > > + pr_debug(" daemon sleep: %us\n", le32_to_cpu(sb->daemon_sleep)); > > pr_debug(" sync size: %llu KB\n", > > (unsigned long long)le64_to_cpu(sb->sync_size)/2); > > - pr_debug("max write behind: %d\n", le32_to_cpu(sb->write_behind)); > > + pr_debug("max write behind: %u\n", le32_to_cpu(sb->write_behind)); > > kunmap_atomic(sb); > > } > > > > @@ -2105,7 +2105,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > > bytes = DIV_ROUND_UP(chunks, 8); > > if (!bitmap->mddev->bitmap_info.external) > > bytes += sizeof(bitmap_super_t); > > - } while (bytes > (space << 9)); > > + } while (bytes > (space << 9) && (chunkshift + BITMAP_BLOCK_SHIFT) < > > + (BITS_PER_BYTE * sizeof(((bitmap_super_t *)0)->chunksize) - 1)); > > } else > > chunkshift = ffz(~chunksize) - BITMAP_BLOCK_SHIFT; > > > > @@ -2150,7 +2151,7 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > > bitmap->counts.missing_pages = pages; > > bitmap->counts.chunkshift = chunkshift; > > bitmap->counts.chunks = chunks; > > - bitmap->mddev->bitmap_info.chunksize = 1 << (chunkshift + > > + bitmap->mddev->bitmap_info.chunksize = 1UL << (chunkshift + > > BITMAP_BLOCK_SHIFT); > > > > blocks = min(old_counts.chunks << old_counts.chunkshift, > > @@ -2176,8 +2177,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, > > bitmap->counts.missing_pages = old_counts.pages; > > bitmap->counts.chunkshift = old_counts.chunkshift; > > bitmap->counts.chunks = old_counts.chunks; > > - bitmap->mddev->bitmap_info.chunksize = 1 << (old_counts.chunkshift + > > - BITMAP_BLOCK_SHIFT); > > + bitmap->mddev->bitmap_info.chunksize = > > + 1UL << (old_counts.chunkshift + BITMAP_BLOCK_SHIFT); > > blocks = old_counts.chunks << old_counts.chunkshift; > > pr_warn("Could not pre-allocate in-memory bitmap for cluster raid\n"); > > break; > > @@ -2534,6 +2535,9 @@ chunksize_store(struct mddev *mddev, const char *buf, size_t len) > > if (csize < 512 || > > !is_power_of_2(csize)) > > return -EINVAL; > > + if (csize >= (1UL << (BITS_PER_BYTE * > > + sizeof(((bitmap_super_t *)0)->chunksize)))) > > + return -EOVERFLOW; > > mddev->bitmap_info.chunksize = csize; > > return len; > > }
On Mon, Oct 17, 2022 at 11:17 AM kernel test robot <lkp@intel.com> wrote: > > Hi Jack, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on song-md/md-next] > [also build test WARNING on linus/master v6.1-rc1 next-20221017] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Jack-Wang/md-bitmap-Fix-bitmap-chunk-size-overflow-issues/20221017-103245 > base: git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git md-next > patch link: https://lore.kernel.org/r/20221014122032.47784-1-jinpu.wang%40ionos.com > patch subject: [PATCH] md/bitmap: Fix bitmap chunk size overflow issues. > config: microblaze-randconfig-r032-20221017 > compiler: microblaze-linux-gcc (GCC) 12.1.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/68927a99dc7197ea076a3b40d876337073f70a58 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Jack-Wang/md-bitmap-Fix-bitmap-chunk-size-overflow-issues/20221017-103245 > git checkout 68927a99dc7197ea076a3b40d876337073f70a58 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/md/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > drivers/md/md-bitmap.c: In function 'chunksize_store': > >> drivers/md/md-bitmap.c:2538:27: warning: left shift count >= width of type [-Wshift-count-overflow] > 2538 | if (csize >= (1UL << (BITS_PER_BYTE * > | ^~ > Thanks will fix in v2. > > vim +2538 drivers/md/md-bitmap.c > > 2523 > 2524 static ssize_t > 2525 chunksize_store(struct mddev *mddev, const char *buf, size_t len) > 2526 { > 2527 /* Can only be changed when no bitmap is active */ > 2528 int rv; > 2529 unsigned long csize; > 2530 if (mddev->bitmap) > 2531 return -EBUSY; > 2532 rv = kstrtoul(buf, 10, &csize); > 2533 if (rv) > 2534 return rv; > 2535 if (csize < 512 || > 2536 !is_power_of_2(csize)) > 2537 return -EINVAL; > > 2538 if (csize >= (1UL << (BITS_PER_BYTE * > 2539 sizeof(((bitmap_super_t *)0)->chunksize)))) > 2540 return -EOVERFLOW; > 2541 mddev->bitmap_info.chunksize = csize; > 2542 return len; > 2543 } > 2544 > > -- > 0-DAY CI Kernel Test Service > https://01.org/lkp
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index bf6dffadbe6f..b266711485a8 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -486,7 +486,7 @@ void md_bitmap_print_sb(struct bitmap *bitmap) sb = kmap_atomic(bitmap->storage.sb_page); pr_debug("%s: bitmap file superblock:\n", bmname(bitmap)); pr_debug(" magic: %08x\n", le32_to_cpu(sb->magic)); - pr_debug(" version: %d\n", le32_to_cpu(sb->version)); + pr_debug(" version: %u\n", le32_to_cpu(sb->version)); pr_debug(" uuid: %08x.%08x.%08x.%08x\n", le32_to_cpu(*(__le32 *)(sb->uuid+0)), le32_to_cpu(*(__le32 *)(sb->uuid+4)), @@ -497,11 +497,11 @@ void md_bitmap_print_sb(struct bitmap *bitmap) pr_debug("events cleared: %llu\n", (unsigned long long) le64_to_cpu(sb->events_cleared)); pr_debug(" state: %08x\n", le32_to_cpu(sb->state)); - pr_debug(" chunksize: %d B\n", le32_to_cpu(sb->chunksize)); - pr_debug(" daemon sleep: %ds\n", le32_to_cpu(sb->daemon_sleep)); + pr_debug(" chunksize: %u B\n", le32_to_cpu(sb->chunksize)); + pr_debug(" daemon sleep: %us\n", le32_to_cpu(sb->daemon_sleep)); pr_debug(" sync size: %llu KB\n", (unsigned long long)le64_to_cpu(sb->sync_size)/2); - pr_debug("max write behind: %d\n", le32_to_cpu(sb->write_behind)); + pr_debug("max write behind: %u\n", le32_to_cpu(sb->write_behind)); kunmap_atomic(sb); } @@ -2105,7 +2105,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, bytes = DIV_ROUND_UP(chunks, 8); if (!bitmap->mddev->bitmap_info.external) bytes += sizeof(bitmap_super_t); - } while (bytes > (space << 9)); + } while (bytes > (space << 9) && (chunkshift + BITMAP_BLOCK_SHIFT) < + (BITS_PER_BYTE * sizeof(((bitmap_super_t *)0)->chunksize) - 1)); } else chunkshift = ffz(~chunksize) - BITMAP_BLOCK_SHIFT; @@ -2150,7 +2151,7 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, bitmap->counts.missing_pages = pages; bitmap->counts.chunkshift = chunkshift; bitmap->counts.chunks = chunks; - bitmap->mddev->bitmap_info.chunksize = 1 << (chunkshift + + bitmap->mddev->bitmap_info.chunksize = 1UL << (chunkshift + BITMAP_BLOCK_SHIFT); blocks = min(old_counts.chunks << old_counts.chunkshift, @@ -2176,8 +2177,8 @@ int md_bitmap_resize(struct bitmap *bitmap, sector_t blocks, bitmap->counts.missing_pages = old_counts.pages; bitmap->counts.chunkshift = old_counts.chunkshift; bitmap->counts.chunks = old_counts.chunks; - bitmap->mddev->bitmap_info.chunksize = 1 << (old_counts.chunkshift + - BITMAP_BLOCK_SHIFT); + bitmap->mddev->bitmap_info.chunksize = + 1UL << (old_counts.chunkshift + BITMAP_BLOCK_SHIFT); blocks = old_counts.chunks << old_counts.chunkshift; pr_warn("Could not pre-allocate in-memory bitmap for cluster raid\n"); break; @@ -2534,6 +2535,9 @@ chunksize_store(struct mddev *mddev, const char *buf, size_t len) if (csize < 512 || !is_power_of_2(csize)) return -EINVAL; + if (csize >= (1UL << (BITS_PER_BYTE * + sizeof(((bitmap_super_t *)0)->chunksize)))) + return -EOVERFLOW; mddev->bitmap_info.chunksize = csize; return len; }