Re: [Update PATCH V3] md: don't unregister sync_thread with reconfig_mutex held

[Guoqing Jiang <guoqing.jiang@linux.dev>]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]



On 5/11/22 2:02 AM, Song Liu wrote:
> On Tue, May 10, 2022 at 5:35 AM Donald Buczek <buczek@molgen.mpg.de> wrote:
>> On 5/10/22 2:09 PM, Guoqing Jiang wrote:
>>>
>>> On 5/10/22 8:01 PM, Donald Buczek wrote:
>>>>> I guess v2 is the best at the moment. I pushed a slightly modified v2 to
>>>>> md-next.
>>>> I think, this can be used to get a double-free from md_unregister_thread.
>>>>
>>>> Please review
>>>>
>>>> https://lore.kernel.org/linux-raid/8312a154-14fb-6f07-0cf1-8c970187cc49@molgen.mpg.de/
>>> That is supposed to be addressed by the second one, pls consider it too.
>> Right, but this has not been pulled into md-next. I just wanted to note, that the current state of md-next has this problem.

Thanks for reminder.

>> If the other patch is taken, too, and works as intended, that would be solved.
>>
>>> [PATCH 2/2] md: protect md_unregister_thread from reentrancy
> Good catch!
>
> Guoqing, current 2/2 doesn't apply cleanly. Could you please resend it on top of
> md-next?

Hmm, no issue from my side.

~/source/md> git am 
0001-md-protect-md_unregister_thread-from-reentrancy.patch
Applying: md: protect md_unregister_thread from reentrancy

~/source/md> git log --oneline |head -5
dc7147a88766 md: protect md_unregister_thread from reentrancy
5a36c493dc82 md: don't unregister sync_thread with reconfig_mutex held
49c3b9266a71 block: null_blk: Improve device creation with configfs
db060f54e0c5 block: null_blk: Cleanup messages
b3a0a73e8a79 block: null_blk: Cleanup device creation and deletion

Anyway, it is attached. I will rebase it to your latest tree if 
something gets wrong.

Thanks,
Guoqing

[-- Attachment #2: 0001-md-protect-md_unregister_thread-from-reentrancy.patch --]
[-- Type: text/x-patch, Size: 1751 bytes --]

From a2da80f62f15023e3fee7a02488c143dfff647b3 Mon Sep 17 00:00:00 2001
From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
Date: Fri, 29 Apr 2022 16:49:09 +0800
Subject: [PATCH 2/2] md: protect md_unregister_thread from reentrancy

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.

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