Re: [PATCH v4 12/25] md/md-llbitmap: track target reshape geometry fields

[sashiko-bot@kernel.org]

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

New issues:
- [High] Target reshape geometry is silently discarded across reboots due to NULL `mddev->pers` during array assembly.
- [High] Reshape geometry tracking fails completely because target personality callbacks are unimplemented.

Pre-existing issues:
- [Critical] Expanded array regions suffer silent data loss during I/O due to mismatched chunkshift indexing.
- [Critical] Use-After-Free (UAF) in timer callback during subsystem teardown due to inadequate synchronization.
--

--- Patch [12]: [PATCH v4 12/25] md/md-llbitmap: track target reshape geometry fields ---
commit fff2be87a121aeeb8a4cb9f28329398a1b07d67a
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: track target reshape geometry fields

Track llbitmap bookkeeping for the target reshape geometry while keeping
a single live bitmap instance. Add the reshape geometry fields, refresh
helper, and update the load and resize paths to keep the target geometry
in sync.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index cf522fae78f8a..37d1312e3eab9 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -430,6 +433,39 @@ static void llbitmap_calculate_chunks(struct mddev *mddev, sector_t blocks,
>  	}
>  }
>  
> +static bool llbitmap_reshaping(struct llbitmap *llbitmap)
> +{
> +	return llbitmap->mddev->reshape_position != MaxSector;
> +}
> +
> +static sector_t llbitmap_personality_sync_size(struct llbitmap *llbitmap,
> +					       bool previous)
> +{
> +	struct mddev *mddev = llbitmap->mddev;
> +
> +	if (!llbitmap_reshaping(llbitmap) || !mddev->private || !mddev->pers ||
> +	    !mddev->pers->bitmap_sync_size)
> +		return llbitmap->sync_size;

[Severity: High]
Does this code handle array assembly correctly across reboots?

During array assembly, md_run() invokes md_bitmap_create() to load the
bitmap before explicitly assigning mddev->pers. Because mddev->pers is still
NULL during this bitmap loading phase, this check will unconditionally
hit the fallback path and return the old pre-reshape llbitmap->sync_size.

Could this cause the target reshape geometry to be completely lost if the
system reboots during a reshape, leading to inconsistent tracking?

[Severity: High]
Are there any RAID personality drivers that actually implement the new
bitmap_sync_size callback?

It appears these callbacks are not yet implemented in any RAID personality
drivers (e.g., raid5, raid1). Consequently, the check for
!mddev->pers->bitmap_sync_size will always evaluate to true, forcing a silent
fallback to the old geometry sizes.

Will this render the newly introduced reshape tracking mechanisms unable
to track the target geometry?

> +	return mddev->pers->bitmap_sync_size(mddev, previous);
> +}
> +
> +static void llbitmap_refresh_reshape(struct llbitmap *llbitmap)
> +{

[ ... ]

> @@ -1289,6 +1327,9 @@ static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  		goto out;
>  
>  	if (reshape) {
> +		llbitmap->reshape_sync_size = blocks;
> +		llbitmap->reshape_chunksize = bitmap_chunksize;
> +		llbitmap->reshape_chunks = chunks;
>  		llbitmap->chunks = max(old_chunks, chunks);

[Severity: High]
This is a pre-existing issue, but does this code scale the chunkshift to match
the updated chunksize?

When a reshape increases array capacity beyond the bitmap's physical limits,
bitmap_chunksize is scaled up. The code updates llbitmap->chunks but seems
to miss updating llbitmap->chunkshift to match the new chunk size.

Later, during I/O operations, llbitmap_start_write() calculates the chunk
index using the stale, smaller chunkshift:

    unsigned long start = offset >> llbitmap->chunkshift;

This produces artificially large chunk indices for data in the expanded region,
which llbitmap_state_machine() then detects as exceeding llbitmap->chunks and
silently drops:

    if (start >= llbitmap->chunks)
        return BitNone;

Could this lead to silent data corruption if writes to the expanded capacity
are never tracked in the bitmap?

>  	} else {
>  		if (blocks < old_blocks && chunks < old_chunks)

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there adequate synchronization when
tearing down the bitmap?

During array destruction, llbitmap_destroy() initiates teardown by deleting
the timer and flushing the workqueue:

    timer_delete_sync(&llbitmap->pending_timer);
    flush_workqueue(md_llbitmap_io_wq);

However, the function doesn't seem to set any shutdown flag. If the background
work md_llbitmap_daemon_fn() is executing or queued when flush_workqueue()
is called, it runs to completion and unconditionally re-arms the timer:

    mod_timer(&llbitmap->pending_timer, ...);

Once the workqueue is flushed, llbitmap_destroy() proceeds to free the
llbitmap structure. The newly re-armed timer can subsequently fire and attempt
to access the freed memory inside llbitmap_pending_timer_fn().

Could this sequence result in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801172519.2982121-1-yukuai@kernel.org?part=12