On 12/10/21 10:16 PM, Donald Buczek wrote:

Dear Guoqing,

On 13.02.21 01:49, Guoqing Jiang wrote:

quoted

Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.

And it could cause deadlock problem for raid5 as follows:

1. process A tried to reap sync thread with reconfig_mutex held after 
echo
    idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper 
layer)
    which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not 
able
    to hold reconfig_mutex.

More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t (local) 


And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.

Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.

  drivers/md/dm-raid.c |  2 +-
  drivers/md/md.c      | 14 +++++++++-----
  drivers/md/md.h      |  2 +-
  3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index cab12b2..0c4cbba 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c

@@ -3668,7 +3668,7 @@ static int raid_message(struct dm_target *ti, 

unsigned int argc, char **argv,
      if (!strcasecmp(argv[0], "idle") || !strcasecmp(argv[0], 
"frozen")) {
          if (mddev->sync_thread) {
              set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-            md_reap_sync_thread(mddev);
+            md_reap_sync_thread(mddev, false);
          }
      } else if (decipher_sync_action(mddev, mddev->recovery) != 
st_idle)
          return -EBUSY;

diff --git a/drivers/md/md.c b/drivers/md/md.c
index ca40942..0c12b7f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -4857,7 +4857,7 @@ action_store(struct mddev *mddev, const char 

*page, size_t len)
                  flush_workqueue(md_misc_wq);
              if (mddev->sync_thread) {
                  set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-                md_reap_sync_thread(mddev);
+                md_reap_sync_thread(mddev, true);
              }
              mddev_unlock(mddev);
          }

@@ -6234,7 +6234,7 @@ static void __md_stop_writes(struct mddev *mddev)

          flush_workqueue(md_misc_wq);
      if (mddev->sync_thread) {
          set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-        md_reap_sync_thread(mddev);
+        md_reap_sync_thread(mddev, true);
      }
        del_timer_sync(&mddev->safemode_timer);

@@ -9256,7 +9256,7 @@ void md_check_recovery(struct mddev *mddev)

               * ->spare_active and clear saved_raid_disk
               */
              set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-            md_reap_sync_thread(mddev);
+            md_reap_sync_thread(mddev, true);
              clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
              clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
              clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);

@@ -9291,7 +9291,7 @@ void md_check_recovery(struct mddev *mddev)

              goto unlock;
          }
          if (mddev->sync_thread) {
-            md_reap_sync_thread(mddev);
+            md_reap_sync_thread(mddev, true);
              goto unlock;
          }
          /* Set RUNNING before clearing NEEDED to avoid

@@ -9364,14 +9364,18 @@ void md_check_recovery(struct mddev *mddev)

  }
  EXPORT_SYMBOL(md_check_recovery);
  -void md_reap_sync_thread(struct mddev *mddev)
+void md_reap_sync_thread(struct mddev *mddev, bool reconfig_mutex_held)
  {
      struct md_rdev *rdev;
      sector_t old_dev_sectors = mddev->dev_sectors;
      bool is_reshaped = false;
        /* resync has finished, collect result */
+    if (reconfig_mutex_held)
+        mddev_unlock(mddev);

If one thread got here, e.g. via action_store( /* "idle" */ ), now 
that the mutex is unlocked, is there anything which would prevent 
another thread getting  here as well, e.g. via the same path?

If not, they both might call

quoted

md_unregister_thread(&mddev->sync_thread);

Which is not reentrant:

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
     * non-existent thread
     */
    spin_lock(&pers_lock);
    *threadp = NULL;
    spin_unlock(&pers_lock);

    kthread_stop(thread->tsk);
    kfree(thread);
}

This might be a preexisting problem, because the call site in 
dm-raid.c, which you updated to `md_reap_sync_thread(mddev, false);`, 
didn't hold the mutex anyway.

Am I missing something? Probably, I do.

Otherwise: Move the deref of threadp in md_unregister_thread() into 
the spinlock scope?

Good point, I think you are right.

And actually pers_lock does extra service to protect accesses to 
mddev->thread (I think it
also suitable for mddev->sync_thread ) when the mutex can't be held. 
Care to send a patch
for it?

Thanks,
Guoqing