Re: [PATCH v3 10/21] md/md-llbitmap: refuse reshape while llbitmap still needs sync

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The newly introduced reshape rejection callback is never invoked by the core MD subsystem, rendering the fix ineffective.
- [Medium] The `llbitmap_reshape_can_start` function caches the bitmap pointer outside of the mutex lock, creating a Time-Of-Check to Time-Of-Use vulnerability that can lead to a Use-After-Free.
- [Medium] The reshape check ignores `BitNeedSyncUnwritten` and `BitSyncingUnwritten`, allowing reshape to start during proactive sync.
--

--- Patch [10]: [PATCH v3 10/21] md/md-llbitmap: refuse reshape while llbitmap still needs sync ---
commit b7405e7962d5eb002f38e66bc1e8cec9737a66ea
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: refuse reshape while llbitmap still needs sync

This commit rejects reshape operations when the llbitmap still contains
NeedSync or Syncing bits. It aims to prevent reshape from starting until
the current llbitmap state has been reconciled.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 55ab2a8a11e5f..f5df82adef640 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -1689,6 +1689,29 @@ static void llbitmap_dirty_bits(struct mddev *mddev, unsigned long s,
>  	llbitmap_state_machine(mddev->bitmap, s, e, BitmapActionStartwrite);
>  }
>  
> +static int llbitmap_reshape_can_start(struct mddev *mddev)
> +{
> +	struct llbitmap *llbitmap = mddev->bitmap;
> +	unsigned long chunk;
> +	int ret = 0;
> +
> +	if (!llbitmap)
> +		return 0;
> +
> +	mutex_lock(&mddev->bitmap_info.mutex);

[Severity: Medium]
Is there a race condition between reading the bitmap pointer and taking the
mutex?

If llbitmap_destroy() executes concurrently, acquires the mutex, sets
mddev->bitmap to NULL, and frees the bitmap while we are checking the pointer
outside the lock, could we end up dereferencing a freed pointer inside the loop
once the lock is acquired here?

> +	for (chunk = 0; chunk < llbitmap->chunks; chunk++) {
> +		enum llbitmap_state state = llbitmap_read(llbitmap, chunk);
> +
> +		if (state == BitNeedSync || state == BitSyncing) {

[Severity: Medium]
Does this condition miss the unwritten sync states?

If BitNeedSyncUnwritten or BitSyncingUnwritten are omitted here, could reshape
be allowed to start during proactive parity synchronization on unwritten
regions, potentially leading to data corruption?

> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&mddev->bitmap_info.mutex);
> +
> +	return ret;
> +}
> +

[ ... ]

> @@ -2011,6 +2034,7 @@ static struct bitmap_operations llbitmap_ops = {
>  	.dirty_bits		= llbitmap_dirty_bits,
>  	.prepare_range		= llbitmap_prepare_range,
>  	.reshape_finish		= llbitmap_reshape_finish,
> +	.reshape_can_start	= llbitmap_reshape_can_start,

[Severity: High]
Is the reshape_can_start callback invoked anywhere in the core MD subsystem?

It doesn't appear that any callers for reshape_can_start are added in this
patch or exist in the current subsystem code. Without a caller, is this
reshape rejection logic effectively dead code?

>  	.write_all		= llbitmap_write_all,
>  
>  	.groups			= md_llbitmap_groups,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785206690.git.yukuai@fygo.io?part=10