Re: [PATCH v2] md/raid5: reject raid4/5 arrays with too few disks

[sashiko-bot@kernel.org]

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

New issues:
- [High] Rejecting RAID5 arrays with fewer than 3 disks breaks explicitly supported 2-disk RAID5 arrays, preventing assembly and RAID1-to-RAID5 reshaping.

Pre-existing issues:
- [Critical] The patch fails to validate `previous_raid_disks` during a reshape, allowing the syzbot panic (`data_disks == 0`) to still be triggered.
- [High] Null pointer dereference during hotplug teardown in raid5_free_percpu() error path.
- [Medium] Memory leak of disk extra_page allocations on the setup_conf() error path.
--

commit 354e4797772beea1449d8c8f033639134da1c7b9
Author: Chen Cheng <chencheng@fnnas.com>

md/raid5: reject raid4/5 arrays with too few disks

This commit rejects RAID4 arrays with fewer than 2 disks and RAID5 arrays
with fewer than 3 disks during setup_conf(). This is intended to prevent
invalid geometries from reaching queue limit setup and causing a panic
due to a zero stripe size.

Closes: https://lore.kernel.org/r/6a778c82.01d0871a.3a0d52.006a.GAE@google.com/

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..4d8bb0cf26055 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7657,9 +7657,12 @@ static struct r5conf *setup_conf(struct mddev *mddev)
>  			mdname(mddev), mddev->new_layout);
>  		return ERR_PTR(-EIO);
>  	}
> -	if (mddev->new_level == 6 && mddev->raid_disks < 4) {
> -		pr_warn("md/raid:%s: not enough configured devices (%d, minimum 4)\n",
> -			mdname(mddev), mddev->raid_disks);
> +	if ((mddev->new_level == 4 && mddev->raid_disks < 2) ||
> +	    (mddev->new_level == 5 && mddev->raid_disks < 3) ||

[Severity: High]
Does this unintentionally break 2-disk RAID5 arrays?

The md driver supports and intentionally creates 2-disk RAID5 arrays during a
RAID1-to-RAID5 reshape. For example, in drivers/md/raid5.c
raid5_takeover_raid1() creates a 2-disk RAID5:

    mddev->new_level = 5;
    mddev->new_layout = ALGORITHM_LEFT_SYMMETRIC;
    mddev->new_chunk_sectors = chunksect;

    ret = setup_conf(mddev);

By unconditionally rejecting mddev->new_level == 5 && mddev->raid_disks < 3
here, doesn't this cause valid reshape operations and assembly of legitimate
2-disk RAID5 arrays to fail with -EINVAL?

> +	    (mddev->new_level == 6 && mddev->raid_disks < 4)) {
> +		pr_warn("md/raid:%s: not enough configured devices (%d, minimum %d)\n",
> +			mdname(mddev), mddev->raid_disks,
> +			mddev->new_level - 2);
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this actually fix the root cause of
the syzbot panic?

Later in setup_conf(), previous_raid_disks is calculated using an unvalidated
delta_disks from the superblock:

    if (mddev->reshape_position == MaxSector)
        conf->previous_raid_disks = mddev->raid_disks;
    else
        conf->previous_raid_disks = mddev->raid_disks - mddev->delta_disks;

An attacker or fuzzed image could bypass the new minimum disk check by
providing a valid raid_disks (e.g., 3 for RAID5) but supplying a positive
delta_disks (e.g., 2), resulting in previous_raid_disks = 1.

Then in raid5_set_limits():

    data_disks = conf->previous_raid_disks - conf->max_degraded;
    stripe = roundup_pow_of_two(data_disks * (mddev->chunk_sectors << 9));

Since max_degraded is 1 for RAID5, data_disks becomes 0 again, and
roundup_pow_of_two(0) will still trigger the exact same division by zero
panic. Should we also be validating delta_disks or previous_raid_disks?

[Severity: Medium]
This is a pre-existing issue, but does the error path out of setup_conf()
leak the disk extra_page allocations?

In setup_conf(), extra pages are allocated using max_disks as the bound:

    for (i = 0; i < max_disks; i++) {
        conf->disks[i].extra_page = alloc_page(GFP_KERNEL);
        if (!conf->disks[i].extra_page)
            goto abort;
    }

If a subsequent initialization step fails, setup_conf() jumps to the abort
label and calls free_conf(). However, free_conf() iterates using
conf->pool_size to free these pages:

    for (i = 0; i < conf->pool_size; i++)
        if (conf->disks[i].extra_page)
            put_page(conf->disks[i].extra_page);

Since conf->pool_size is zero-initialized in setup_conf() and only populated
later in grow_stripes(), doesn't the cleanup loop terminate immediately and
leak all the successfully allocated pages?

[Severity: High]
This is a pre-existing issue, but does the setup_conf() error path trigger a
NULL pointer dereference during hotplug teardown?

Earlier in setup_conf(), raid5_alloc_percpu() is called:

    conf->percpu = alloc_percpu(struct raid5_percpu);
    if (!conf->percpu)
        return -ENOMEM;

    err = cpuhp_state_add_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
    if (!err) {
        ...

If cpuhp_state_add_instance() fails (for example, due to a memory allocation
failure in raid456_cpu_up_prepare()), it returns an error but leaves
conf->percpu allocated and conf->node unhashed.

When setup_conf() handles the error by jumping to abort and calling
free_conf(), it invokes raid5_free_percpu():

    if (!conf->percpu)
        return;
    
    cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);

Because conf->percpu is non-NULL, cpuhp_state_remove_instance() is called.
The CPU hotplug core then calls hlist_del(&conf->node), which dereferences
the NULL pprev pointer of the unhashed list node. Can this crash the system
under memory pressure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905104323.2202902-1-chencheng@fnnas.com?part=1