Re: [PATCH 1/4] md/md-bitmap: mask sb->state on load to drop runtime-only bits

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The lockless bitmap implementation (llbitmap) remains vulnerable to the exact same persistent WRITE_ERROR bug that this commit fixes for the standard bitmap.
- [High] Internal bitmap pages allocated with alloc_page() are leaked during array teardown when CONFIG_MD_BITMAP_FILE is enabled.
- [High] Cluster superblock flags leak across `re_read` loop iterations, causing spurious state inheritance from node 0.
--

--- Patch [1]: [PATCH 1/4] md/md-bitmap: mask sb->state on load to drop runtime-only bits ---
commit f4f8e55ab2fda3c56252ec133561c13874aeb19a
Author: Mykola Marzhan <[email protected]>

md/md-bitmap: mask sb->state on load to drop runtime-only bits

This patch masks the loaded state to BITMAP_STALE in md_bitmap_read_sb() so
any runtime-only bits like BITMAP_WRITE_ERROR are dropped. This fixes an
issue where an array refuses to assemble if it previously hit a transient
bitmap write error under an older kernel.

> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 0f02e2956398d..4dd3fc2a9e15b 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -931,8 +931,13 @@ static int md_bitmap_read_sb(struct bitmap *bitmap)
>  		}
>  	}
>  
> -	/* assign fields using values from superblock */
> -	bitmap->flags |= le32_to_cpu(sb->state);
> +	/*
> +	 * Only BITMAP_STALE is meaningful to this loader from sb->state:
> +	 * WRITE_ERROR and DAEMON_BUSY are kernel-runtime signals (kernels
> +	 * before v4.15 persisted WRITE_ERROR), FIRST_USE/CLEAN are
> +	 * llbitmap-only, and HOSTENDIAN is derived from sb->version below.
> +	 */
> +	bitmap->flags |= le32_to_cpu(sb->state) & BIT(BITMAP_STALE);

[Severity: High]
This isn't a bug introduced by this patch, but does this bitwise OR cause
cluster superblock flags to leak across re_read loop iterations?

In md_bitmap_read_sb(), when nodes > 0, the code reads offset 0 initially and
accumulates flags into bitmap->flags. It then computes a new offset for its
own cluster_slot and jumps back to the re_read label, but bitmap->flags is
never cleared.

Could this cause a local node to spuriously inherit BITMAP_STALE or
BITMAP_HOSTENDIAN from node 0, incorrectly forcing a full array recovery?

[Severity: High]
This is a pre-existing issue, but does the lockless bitmap implementation in
drivers/md/md-llbitmap.c remain vulnerable to the exact same WRITE_ERROR bug
this patch fixes for standard bitmaps?

In llbitmap_read_sb():

    llbitmap->flags = le32_to_cpu(sb->state);

Since it reads the superblock state entirely unmasked, wouldn't a single
instance of on-disk corruption or transient error set BITMAP_WRITE_ERROR
permanently and force a full resync on every restart?

[Severity: High]
This is also a pre-existing issue, but do internal bitmap pages leak during
array teardown when CONFIG_MD_BITMAP_FILE is enabled?

In md_bitmap_file_unmap(), free_buffers() is called for each page:

drivers/md/md-bitmap.c:free_buffers() {
    ...
    if (!PagePrivate(page))
        return;
    ...
}

For internal bitmaps, reads and writes use sync_page_io() without buffer
heads, so PagePrivate is never set. Does this early return skip put_page()
and permanently leak all dynamically allocated metadata pages whenever the
array is stopped or the bitmap is resized?

>  	if (le32_to_cpu(sb->version) == BITMAP_MAJOR_HOSTENDIAN)
>  		set_bit(BITMAP_HOSTENDIAN, &bitmap->flags);
>  	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1