Re: [PATCH v4 03/25] md/md-llbitmap: only end fully synced chunks

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

Pre-existing issues:
- [High] Mutually re-arming timer and workqueue are not safely torn down in llbitmap_destroy(), leading to a Use-After-Free.
- [High] Missing wait for behind writes in llbitmap_destroy() leads to a NULL pointer dereference or UAF when inflight behind writes complete.
--

--- Patch [3]: [PATCH v4 03/25] md/md-llbitmap: only end fully synced chunks ---

commit e6e3673364dedea74baaa7ed43c3fb7a3c397e90
Author: Yu Kuai [off-list ref]

md/md-llbitmap: only end fully synced chunks
    
llbitmap_cond_end_sync() is called with the sync thread's current sector,
which is an exclusive progress boundary where the chunk containing it can
still be in progress. The old code converted that sector directly to the last
bit, which allowed in-progress chunks to be mistakenly marked as synced. This
patch fixes the issue by rounding the sector down to the nearest chunk
boundary and ending only fully synced chunks.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a preexisting
issue where the mutually re-arming timer and workqueue are not safely torn
down in llbitmap_destroy().

drivers/md/md-llbitmap.c:llbitmap_destroy() {
...
	timer_delete_sync(&llbitmap->pending_timer);
	flush_workqueue(md_llbitmap_io_wq);
...
}

If md_llbitmap_daemon_fn() is executing concurrently, it finishes its loop,
sets restart to true, and unconditionally re-arms the timer without checking
if the bitmap is being destroyed:

drivers/md/md-llbitmap.c:md_llbitmap_daemon_fn() {
...
	if (restart)
		mod_timer(&llbitmap->pending_timer,
			  jiffies + llbitmap->mddev->bitmap_info.daemon_sleep * HZ);
...
}

Can this lead to a Use-After-Free when flush_workqueue() completes and the
llbitmap struct is freed, leaving an active timer pointing to freed memory?


[Severity: High]
This is a pre-existing issue, but I also noticed missing synchronization for
inflight behind writes before destroying the bitmap.

drivers/md/md-llbitmap.c:llbitmap_destroy() {
	struct llbitmap *llbitmap = mddev->bitmap;
...
}

The destroy function does not wait for inflight behind writes to complete
(atomic_read(&llbitmap->behind_writes) > 0). The caller then sets
mddev->bitmap_ops to NULL.

If an inflight behind write later completes in raid1.c:

drivers/md/raid1.c:close_write() {
...
	if (test_bit(R1BIO_BehindIO, &r1_bio->state))
		mddev->bitmap_ops->end_behind_write(mddev);
...
}

Does this cause a NULL pointer dereference (or UAF) since mddev->bitmap_ops
has been cleared concurrently?

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