Re: [PATCH v3 3/3] md/raid5: fix NULL pointer dereference in raid5_free_percpu

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

Pre-existing issues:
- [High] Missing workqueue flush before freeing worker structures during array teardown.
--

commit 7455143ee1b52af0e50fb9ef4f12b3a32d0dc0c2
Author: Cao Guanghui <[email protected]>

    md/raid5: fix NULL pointer dereference in raid5_free_percpu
    
    If cpuhp_state_add_instance() fails in raid5_alloc_percpu(), conf->node is
    never added to the cpuhp instance list and its pprev remains NULL. This
    patch guards the removal with hlist_unhashed_lockless() so that the cpuhp
    instance is only removed if it was actually added, preventing a kernel panic.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index e481c48b45093..31417c91ab248 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7548,7 +7548,8 @@ static void raid5_free_percpu(struct r5conf *conf)
>  	if (!conf->percpu)
>  		return;
>  
> -	cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
> +	if (!hlist_unhashed_lockless(&conf->node))
> +		cpuhp_state_remove_instance(CPUHP_MD_RAID5_PREPARE, &conf->node);
>  	free_percpu(conf->percpu);
>  }

While reviewing the array teardown path modified here, I noticed a separate
issue in the nearby free_thread_groups() function:

drivers/md/raid5.c:free_thread_groups() {
    ...
	if (conf->worker_groups)
		kfree(conf->worker_groups[0].workers);
	kfree(conf->worker_groups);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing issue.

Does this sequence miss a workqueue flush before freeing the worker structures?

During array teardown in md_stop(), raid5_quiesce() waits for active_stripes
to reach 0. This decrement and the subsequent wake_up(&conf->wait_for_quiescent)
can occur inside release_inactive_stripe_list(). This is called by
handle_active_stripes() (executed by raid5_do_work() on the worker thread)
after temporarily dropping conf->device_lock.

Once woken, raid5_quiesce() finishes and teardown proceeds to
free_thread_groups(). This function frees the memory backing worker->work
without calling flush_workqueue() or cancel_work_sync().

Meanwhile, the worker thread in raid5_do_work() could still be executing its
trailing loop and workqueue core cleanup code. Could this result in a
use-after-free of the worker structures, leading to memory corruption or a
kernel panic?

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