Re: [PATCH 2/6] md/raid5: size stripe-cache hash locks from the CPU count

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

New issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition in `nr_stripe_hash_locks` module parameter evaluation can lead to undefined behavior and a potential kernel panic during array creation.

Pre-existing issues:
- [High] The worker threads (`r5worker->work`) are not explicitly canceled or flushed during array teardown, which can lead to a Use-After-Free.
--

--- Patch [2]: [PATCH 2/6] md/raid5: size stripe-cache hash locks from the CPU count ---
commit bae2376a7fe52c6b3de7d64514b554d3101957fe
Author: Hiroshi Nishida <[email protected]>

md/raid5: size stripe-cache hash locks from the CPU count

This commit modifies the stripe cache hash to use a per-array dynamic count
of spinlocks instead of a fixed 8, automatically sizing it based on the
online CPU count to reduce lock contention on many-core systems.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d8807114a6932..7f72981121fd5 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -7448,6 +7453,9 @@ static void free_conf(struct r5conf *conf)
>  	kfree(conf->disks);
>  	bioset_exit(&conf->bio_split);
>  	kfree(conf->stripe_hashtbl);
> +	kfree(conf->hash_locks);
> +	kfree(conf->inactive_list);
> +	kfree(conf->temp_inactive_list);
>  	kfree(conf->pending_data);

[Severity: High]
This is a pre-existing issue, but looking at the teardown sequence, could
the worker threads access freed memory here?

During array teardown, md_stop() eventually causes a wait for
active_stripes to reach 0. In a worker thread executing raid5_do_work(),
active_stripes drops to 0 when handle_active_stripes() finishes releasing
the final stripes.

At this point, raid5_quiesce() wakes up and teardown proceeds to call
free_thread_groups() which frees the worker structures without calling
flush_workqueue() or cancel_work_sync().

The still-executing worker thread could then drop its lock and continue
to execute flush_deferred_bios() and r5l_flush_stripe_to_raid() using the
freed structures.

Should the worker threads be explicitly canceled or flushed before the
structures are freed?

[ ... ]
> @@ -7565,6 +7573,38 @@ static struct r5conf *setup_conf(struct mddev *mddev)
>  	if (conf == NULL)
>  		goto abort;
>  
> +	/*
> +	 * Size the stripe cache hash-lock striping per array.  The
> +	 * nr_stripe_hash_locks module parameter selects the count: 0 (the
> +	 * default) auto-sizes it from the online CPU count so busier machines
> +	 * stripe the locks wider, while a non-zero value overrides that.  The
> +	 * result is rounded up to a power of two (the hash uses it as a
> +	 * bitmask) and capped at NR_STRIPE_HASH_LOCKS_MAX; the auto path also
> +	 * keeps at least the historical NR_STRIPE_HASH_LOCKS_DEFAULT so small
> +	 * systems are unchanged.  Allocating per array means an untuned system
> +	 * uses no more memory than before.
> +	 */
> +	if (nr_stripe_hash_locks == 0)
> +		conf->nr_hash_locks = clamp_t(unsigned int,
> +				roundup_pow_of_two(num_online_cpus()),
> +				NR_STRIPE_HASH_LOCKS_DEFAULT,
> +				NR_STRIPE_HASH_LOCKS_MAX);
> +	else
> +		conf->nr_hash_locks = roundup_pow_of_two(
> +				min_t(unsigned int, nr_stripe_hash_locks,
> +				      NR_STRIPE_HASH_LOCKS_MAX));

[Severity: High]
Could this be vulnerable to a time-of-check to time-of-use race if the
sysfs parameter is modified concurrently?

If the module parameter is changed from a non-zero value to 0 right after
the check:

Thread 1 in setup_conf():
    if (nr_stripe_hash_locks == 0)
        // check fails, proceeds to else block

Thread 2 writing to sysfs:
    nr_stripe_hash_locks = 0;

Thread 1 inside the else block:
    conf->nr_hash_locks = roundup_pow_of_two(min_t(..., 0, ...));

In this scenario, roundup_pow_of_two() would evaluate to 0. When kcalloc()
is subsequently called, it would return the zero size pointer placeholder
value (16).

The safety check testing for a null pointer would fail to catch this since
16 is non-null, and the kernel could panic when spin_lock_init()
dereferences it.

Would it be safer to read the parameter into a local variable first?

    unsigned int locks = READ_ONCE(nr_stripe_hash_locks);
    if (locks == 0)
        conf->nr_hash_locks = clamp_t(...);
    else
        conf->nr_hash_locks = roundup_pow_of_two(...);

> +	conf->hash_locks_mask = conf->nr_hash_locks - 1;
> +	conf->hash_locks = kcalloc(conf->nr_hash_locks,
> +				   sizeof(*conf->hash_locks), GFP_KERNEL);

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