Re: [PATCH v2 04/20] md/md-llbitmap: track bitmap sync_size explicitly

[sashiko-bot@kernel.org]

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

New issues:
- [High] Data race on `llbitmap->sync_size` between `llbitmap_resize` and `llbitmap_update_sb` can result in a corrupted superblock.

Pre-existing issues:
- [Critical] Improper teardown order in `llbitmap_destroy()` allows the asynchronous workqueue to unsafely re-arm the timer, leading to a Use-After-Free.
--

commit 4f14b77c13988c93db9a3db0e4720c69a8ac613c
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: track bitmap sync_size explicitly

Track llbitmap's own sync_size instead of always using
mddev->resync_max_sectors directly. This is the minimal bookkeeping
needed before llbitmap can track old and new reshape geometry
independently.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index f0c20afa354eb..e3500a1a52c07 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -1153,6 +1161,7 @@ static int llbitmap_resize(struct mddev *mddev, sector_t blocks, int chunksize)
>  	llbitmap->chunkshift = ffz(~chunksize);
>  	llbitmap->chunksize = chunksize;
>  	llbitmap->chunks = chunks;
> +	llbitmap->sync_size = blocks;
>  
>  	return 0;
>  }

[ ... ]

> @@ -1526,7 +1535,7 @@ static void llbitmap_update_sb(void *data)
>  	sb->events = cpu_to_le64(mddev->events);
>  	sb->state = cpu_to_le32(llbitmap->flags);
>  	sb->chunksize = cpu_to_le32(llbitmap->chunksize);
> -	sb->sync_size = cpu_to_le64(mddev->resync_max_sectors);
> +	sb->sync_size = cpu_to_le64(llbitmap->sync_size);

[Severity: High]
Can this read of llbitmap->sync_size cause a torn read on 32-bit systems?

While llbitmap_resize() updates this 64-bit value while holding
mddev->bitmap_info.mutex and reconfig_mutex, md_update_sb() calls
llbitmap_update_sb() without holding bitmap_info.mutex.

If a torn read occurs during concurrent array resize operations, could the
superblock be written with a corrupted sync_size? This might lead to array
assembly failures or out-of-bounds accesses during subsequent operations.

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does the teardown sequence in
llbitmap_destroy() have a use-after-free risk for the timer?

Looking at the current teardown order in llbitmap_destroy():

llbitmap_destroy() {
        ...
        timer_delete_sync(&llbitmap->pending_timer);
        flush_workqueue(md_llbitmap_io_wq);
        ...
}

If md_llbitmap_daemon_fn() is running during the flush_workqueue() call,
it might attempt to re-arm the timer:

md_llbitmap_daemon_fn() {
        ...
        if (restart)
                mod_timer(&llbitmap->pending_timer, jiffies + ...);
}

Could the asynchronous workqueue unsafely re-arm the timer here, allowing
it to resurrect after timer_delete_sync() and execute after the llbitmap
structure has been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782282042.git.yukuai@kernel.org?part=4