Re: [PATCH] md/bitmap: don't set sb values if can't pass sanity check
From: Guoqing Jiang <hidden>
Date: 2022-03-28 00:43:50
On 3/25/22 10:52 AM, Heming Zhao wrote:
If bitmap area contains invalid data, kernel may crash or mdadm triggers FPE (Floating exception) This is cluster-md speical bug. In non-clustered env, mdadm will handle broken metadata case. In clustered array, only kernel space handles bitmap slot info. But even this bug only happened in clustered env, current sanity check is wrong, the code should be changed. How to trigger: (faulty injection) dd if=/dev/zero bs=1M count=3 oflag=direct of=/dev/sda dd if=/dev/zero bs=1M count=3 oflag=direct of=/dev/sdb mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda /dev/sdb mdadm -Ss echo aaa > magic.txt == below modifying slot 2 bitmap data == dd if=magic.txt of=/dev/sda seek=16384 bs=1 count=3 <== destory magic dd if=/dev/zero of=/dev/sda seek=16436 bs=1 count=4 <== ZERO chunksize mdadm -A /dev/md0 /dev/sda /dev/sdb == kernel crash. mdadm reports FPE ==
Thanks, could you also share the crash log to make people understand it better?
quoted hunk ↗ jump to hunk
Signed-off-by: Heming Zhao <redacted> --- drivers/md/md-bitmap.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index bfd6026d7809..f6dcdb3683bf 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c@@ -635,19 +635,6 @@ static int md_bitmap_read_sb(struct bitmap *bitmap) err = -EINVAL; sb = kmap_atomic(sb_page); - chunksize = le32_to_cpu(sb->chunksize); - daemon_sleep = le32_to_cpu(sb->daemon_sleep) * HZ; - write_behind = le32_to_cpu(sb->write_behind); - sectors_reserved = le32_to_cpu(sb->sectors_reserved); - /* Setup nodes/clustername only if bitmap version is - * cluster-compatible - */ - if (sb->version == cpu_to_le32(BITMAP_MAJOR_CLUSTERED)) { - nodes = le32_to_cpu(sb->nodes); - strlcpy(bitmap->mddev->bitmap_info.cluster_name, - sb->cluster_name, 64); - } - /* verify that the bitmap-specific fields are valid */ if (sb->magic != cpu_to_le32(BITMAP_MAGIC)) reason = "bad magic";@@ -668,6 +655,19 @@ static int md_bitmap_read_sb(struct bitmap *bitmap) goto out; } + chunksize = le32_to_cpu(sb->chunksize); + daemon_sleep = le32_to_cpu(sb->daemon_sleep) * HZ; + write_behind = le32_to_cpu(sb->write_behind); + sectors_reserved = le32_to_cpu(sb->sectors_reserved); + /* Setup nodes/clustername only if bitmap version is + * cluster-compatible + */
I suppose kernel should print the "reason" if md failed to verify bitmap sb. And at it, pls change the comment style to /* * */
quoted hunk ↗ jump to hunk
+ if (sb->version == cpu_to_le32(BITMAP_MAJOR_CLUSTERED)) { + nodes = le32_to_cpu(sb->nodes); + strlcpy(bitmap->mddev->bitmap_info.cluster_name, + sb->cluster_name, 64); + } + /* keep the array size field of the bitmap superblock up to date */ sb->sync_size = cpu_to_le64(bitmap->mddev->resync_max_sectors);@@ -700,9 +700,9 @@ static int md_bitmap_read_sb(struct bitmap *bitmap) out: kunmap_atomic(sb); - /* Assigning chunksize is required for "re_read" */ - bitmap->mddev->bitmap_info.chunksize = chunksize; if (err == 0 && nodes && (bitmap->cluster_slot < 0)) { + /* Assigning chunksize is required for "re_read" */ + bitmap->mddev->bitmap_info.chunksize = chunksize; err = md_setup_cluster(bitmap->mddev, nodes); if (err) { pr_warn("%s: Could not setup cluster service (%d)\n",@@ -717,10 +717,12 @@ static int md_bitmap_read_sb(struct bitmap *bitmap) out_no_sb: if (test_bit(BITMAP_STALE, &bitmap->flags)) bitmap->events_cleared = bitmap->mddev->events; - bitmap->mddev->bitmap_info.chunksize = chunksize; - bitmap->mddev->bitmap_info.daemon_sleep = daemon_sleep; - bitmap->mddev->bitmap_info.max_write_behind = write_behind; - bitmap->mddev->bitmap_info.nodes = nodes; + if (err == 0) { + bitmap->mddev->bitmap_info.chunksize = chunksize; + bitmap->mddev->bitmap_info.daemon_sleep = daemon_sleep; + bitmap->mddev->bitmap_info.max_write_behind = write_behind; + bitmap->mddev->bitmap_info.nodes = nodes; + } if (bitmap->mddev->bitmap_info.space == 0 || bitmap->mddev->bitmap_info.space > sectors_reserved) bitmap->mddev->bitmap_info.space = sectors_reserved;
Generally, I am fine with the change. Thanks, Guoqing