Re: [PATCH] md/bitmap: resume array on backlog_store() error path

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

Pre-existing issues:
- [Critical] Silent allocation failures in `mddev_create_serial_pool()` lead to NULL pointer dereferences and silent data corruption.
- [High] Cross-ioctl and error path usage of `mddev_suspend()` in `dm-raid` leaks `PF_MEMALLOC_NOIO` task flag.
--

--- Patch [1]: [PATCH] md/bitmap: resume array on backlog_store() error path ---
Hi,

Thank you for the patch. I noticed a few pre-existing issues in the related
code paths that might be worth addressing.

commit 5425190104cdbeefa8ae0eda1f825b462b6535ca
Author: Chen Cheng <[email protected]>

md/bitmap: resume array on backlog_store() error path
    
backlog_store() suspends the array before checking whether a write-mostly
device exists. If no such device exists, the error path only unlocks
reconfig_mutex and leaves the array suspended, blocking subsequent I/O.
Use mddev_unlock_and_resume() to release both states.

> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 0f02e2956398d..c90fa1e106e26 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2862,7 +2862,7 @@ backlog_store(struct mddev *mddev, const char *buf, size_t len)
>  	if (!has_write_mostly) {
>  		pr_warn_ratelimited("%s: can't set backlog, no write mostly device available\n",
>  				    mdname(mddev));
> -		mddev_unlock(mddev);
> +		mddev_unlock_and_resume(mddev);

[Severity: High]
This isn't a bug introduced by this patch, but since this patch fixes an error
path missing a resume, does this same missing resume problem exist in
dm-raid.c?

In drivers/md/dm-raid.c:raid_postsuspend(), the code suspends the mddev and
acquires the PF_MEMALLOC_NOIO flag, but returns to userspace without resuming:

    mddev_suspend(&rs->md, false);

If a user-space process issues a DM_DEV_SUSPEND ioctl, this leaves the flag
set. When DM_DEV_RESUME is later issued, potentially by a different task,
raid_resume() calls mddev_resume(), which applies the saved flags to the
new task.

Also, in raid_ctr(), if setup fails, the error path skips the restore
completely and permanently leaks the flag:

bad_unlock:
    md_stop(&rs->md);
    mddev_unlock(&rs->md);

Could this permanently leak the PF_MEMALLOC_NOIO task flag and cause
memory reclaim deadlocks?

[Severity: Critical]
This is a pre-existing issue, but further down in backlog_store(), is it
possible for mddev_create_serial_pool() to fail silently?

drivers/md/md-bitmap.c:backlog_store() {
    ...
    } else if (backlog && !mddev->serial_info_pool) {
        /* serial_info_pool is needed since backlog is not zero */
        rdev_for_each(rdev, mddev)
            mddev_create_serial_pool(mddev, rdev);
    }
    ...
}

Since mddev_create_serial_pool() returns void, it hides memory allocation
failures. If it fails for a disk in this loop, that device is left without
initialization. Does this silently bypass write-behind serialization for that
disk, leading to overlapping writes and silent data corruption?

Similarly, if MD_SERIALIZE_POLICY is active, check_and_add_serial() in
drivers/md/raid1.c will unconditionally dereference rdev->serial:

    int idx = sector_to_idx(r1_bio->sector);
    struct serial_in_rdev *serial = &rdev->serial[idx];
    struct serial_info *head_si;

    spin_lock_irqsave(&serial->serial_lock, flags);

Can this cause a NULL pointer dereference for devices that failed
initialization?

>  		return -EINVAL;
>  	}
>

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