Re: [PATCH 2/2] md: protect md_unregister_thread from reentrancy

[Song Liu <song@kernel.org>]

On Thu, May 5, 2022 at 1:18 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>
> Generally, the md_unregister_thread is called with reconfig_mutex, but
> raid_message in dm-raid doesn't hold reconfig_mutex to unregister thread,
> so md_unregister_thread can be called simulitaneously from two call sites
> in theory.

Can we add lock/unlock into raid_message? Are there some constraints here?

Thanks,
Song

>
> Then after previous commit which remove the protection of reconfig_mutex
> for md_unregister_thread completely, the potential issue could be worse
> than before.
>
> Let's take pers_lock at the beginning of function to ensure reentrancy.
>
> Reported-by: Donald Buczek <buczek@molgen.mpg.de>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/md/md.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index a70e7f0f9268..c401e063bec8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7962,17 +7962,22 @@ EXPORT_SYMBOL(md_register_thread);
>
>  void md_unregister_thread(struct md_thread **threadp)
>  {
> -       struct md_thread *thread = *threadp;
> -       if (!thread)
> -               return;
> -       pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
> -       /* Locking ensures that mddev_unlock does not wake_up a
> +       struct md_thread *thread;
> +
> +       /*
> +        * Locking ensures that mddev_unlock does not wake_up a
>          * non-existent thread
>          */
>         spin_lock(&pers_lock);
> +       thread = *threadp;
> +       if (!thread) {
> +               spin_unlock(&pers_lock);
> +               return;
> +       }
>         *threadp = NULL;
>         spin_unlock(&pers_lock);
>
> +       pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
>         kthread_stop(thread->tsk);
>         kfree(thread);
>  }
> --
> 2.31.1
>