Re: [PATCH v6 1/3] md: suspend array when sync_action=reshape

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

Pre-existing issues:
- [Critical] Concurrent calls to mddev_suspend() corrupt current->flags due to a shared mddev->noio_flag.
- [High] TOCTOU race on mddev->pers in action_store leading to NULL pointer dereference.
- [High] Unprotected access to mddev->pers inside mddev_suspend leading to TOCTOU and potential Use-After-Free.
--

commit 480ff61edd64974d75f3192274f5151dada9f98c
Author: Chen Cheng <[email protected]>

md: suspend array when sync_action=reshape

This patch modifies action_store() to suspend and lock the array
before starting a reshape operation, and resume it upon exit. This
prevents new requests from allocating objects from an old pool while
the array is being reconfigured.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd5..1377c407614c6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5263,21 +5263,25 @@ action_store(struct mddev *mddev, const char *page, size_t len)
>  	if (!mddev->pers || !mddev->pers->sync_request)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does checking mddev->pers locklessly
here risk a NULL pointer dereference later in the function?

A process writing "reshape" to sync_action locklessly verifies
mddev->pers != NULL here. If it is preempted or blocks in flush_work(),
a concurrent write to array_state could stop the array, taking
reconfig_mutex and calling do_md_stop(), which sets mddev->pers = NULL.

When the first process resumes, it acquires reconfig_mutex via
mddev_lock() and calls mddev_start_reshape() without re-verifying
mddev->pers:

mddev_start_reshape() {
    if (mddev->pers->start_reshape == NULL)
    ...
}

Could this cause a kernel panic if do_md_stop() has cleared mddev->pers
concurrently?

>  
> +	action = md_sync_action_by_name(page);
>  retry:
>  	if (work_busy(&mddev->sync_work))
>  		flush_work(&mddev->sync_work);
>  
> -	ret = mddev_lock(mddev);
> +	ret = (action == ACTION_RESHAPE) ?
> +		mddev_suspend_and_lock(mddev) :
> +		mddev_lock(mddev);

[Severity: Critical]
This isn't a bug introduced by this patch, but does exposing
mddev_suspend_and_lock() to concurrent sysfs writes via ACTION_RESHAPE
risk corrupting current->flags?

If two processes concurrently write "reshape" to the sync_action sysfs
file, they both execute mddev_suspend_and_lock() -> mddev_suspend()
concurrently without external serialization.

The first process increments mddev->suspended from 0 to 1 and saves
its task-local NOIO state into the shared mddev->noio_flag:

mddev_suspend() {
    ...
    if (!mddev->suspended++) {
        ...
        mddev->noio_flag = memalloc_noio_save();
    ...
}

The second process increments it from 1 to 2 and skips saving its state.

Upon completion, the first process calls mddev_resume(), decrements the
count to 1, and returns without restoring its state. The second process
calls mddev_resume(), decrements the count to 0, and restores the first
process's state onto itself:

__mddev_resume() {
    ...
    if (--mddev->suspended)
        return;
    ...
    memalloc_noio_restore(mddev->noio_flag);
}

Can this cause a permanent leak of the PF_MEMALLOC_NOIO flag for the
first thread and incorrectly apply it to the last thread?

[Severity: High]
This is also a pre-existing issue in mddev_suspend(), but does calling it
here before acquiring reconfig_mutex allow an unprotected access to
mddev->pers?

If a process writes "reshape", it invokes mddev_suspend() which evaluates
mddev->pers without holding reconfig_mutex or mddev->lock:

mddev_suspend() {
    ...
    if (mddev->pers && mddev->pers->prepare_suspend &&
        reshape_interrupted(mddev))
        mddev->pers->prepare_suspend(mddev);
    ...
}

Concurrently, another process calling do_md_stop() could set
mddev->pers = NULL and drop the personality module reference. If
mddev_suspend() checks mddev->pers, but the module is unloaded before
calling mddev->pers->prepare_suspend(), could this result in a NULL
pointer dereference or execution of unmapped memory?

>  	if (ret)
>  		return ret;

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