Re: [PATCH] md: fix rcu protection in md_wakeup_thread

["Zhou, Yun" <yun.zhou@windriver.com>]

Hi Kuai,

On 10/15/25 14:58, Yu Kuai wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender 
> and know the content is safe.
>
> Hi,
>
> 在 2025/10/15 14:35, Paul Menzel 写道:
>> Dear Yun,
>>
>>
>> Thank you for your patch.
>>
>> Am 15.10.25 um 07:59 schrieb Yun Zhou:
>>> We attempted to use RCU to protect the pointer 'thread', but directly
>>> passed the value when calling md_wakeup_thread(). This means that the
>>> RCU pointer has been acquired before rcu_read_lock(), which renders
>>> rcu_read_lock() ineffective and could lead to a use-after-free.
>>
>> Could you elaborate a little more – especially as nobody has noticed
>> this so far since v6.5-rc1?
>>
>
> This looks correct, memory dereference should be protected by rcu, while
> in fact this is done before function parameter passing.
>
> However, in most places, a null check should be enough because the md
> thread can't be unregistered concurrently, that's probably no one ever
> meet the problem.
>
> However, for the modification, I'll prefer a new marcon like following,
> so that you don't need to update all the callers:
>
This method only changes a little code. But it holds the lock before the 
function jumps, which seems to violate the principle of minimizing the 
critical area.
> Thanks,
> Kuai
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1de550108756..d48ee1b50d27 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8384,11 +8384,10 @@ static void md_wakeup_thread_directly(struct
> md_thread __rcu *thread)
>         rcu_read_unlock();
>  }
>
> -void md_wakeup_thread(struct md_thread __rcu *thread)
> +void __md_wakeup_thread(struct md_thread __rcu *thread)
>  {
>         struct md_thread *t;
>
> -       rcu_read_lock();
>         t = rcu_dereference(thread);
>         if (t) {
>                 pr_debug("md: waking up MD thread %s.\n", t->tsk->comm);
> @@ -8396,9 +8395,8 @@ void md_wakeup_thread(struct md_thread __rcu 
> *thread)
>                 if (wq_has_sleeper(&t->wqueue))
>                         wake_up(&t->wqueue);
>         }
> -       rcu_read_unlock();
>  }
> -EXPORT_SYMBOL(md_wakeup_thread);
> +EXPORT_SYMBOL(__md_wakeup_thread);
>
>  struct md_thread *md_register_thread(void (*run) (struct md_thread *),
>                 struct mddev *mddev, const char *name)
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 1979c2d4fe89..9ec62afc2a7d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -882,6 +882,12 @@ struct md_io_clone {
>
>  #define THREAD_WAKEUP  0
>
> +#define md_wakeup_thread(thread) do {                          \
> +       rcu_read_lock();                                        \
> +       __md_wakeup_thread(thread);                             \
> +       rcu_read_unlock();                                      \
> +} while (0)
> +
>  static inline void safe_put_page(struct page *p)
>  {
>         if (p) put_page(p);
> @@ -895,7 +901,7 @@ extern struct md_thread *md_register_thread(
>         struct mddev *mddev,
>         const char *name);
>  extern void md_unregister_thread(struct mddev *mddev, struct md_thread
> __rcu **threadp);
> -extern void md_wakeup_thread(struct md_thread __rcu *thread);
> +extern void __md_wakeup_thread(struct md_thread __rcu *thread);
>  extern void md_check_recovery(struct mddev *mddev);
>  extern void md_reap_sync_thread(struct mddev *mddev);
>  extern enum sync_action md_sync_action(struct mddev *mddev);
>
Regards,
Yun