From ac619781967bd5663c29606246b50dbebd8b3473 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:16 +0800 Subject: [PATCH 01/16] md: use separate work_struct for md_start_sync() It's a little weird to borrow 'del_work' for md_start_sync(), declare a new work_struct 'sync_work' for md_start_sync(). Signed-off-by: Yu Kuai Reviewed-by: Xiao Ni Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-2-yukuai1@huaweicloud.com --- drivers/md/md.c | 10 ++++++---- drivers/md/md.h | 5 ++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index a104a025084d..fb9aae4accb0 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -631,13 +631,13 @@ void mddev_put(struct mddev *mddev) * flush_workqueue() after mddev_find will succeed in waiting * for the work to be done. */ - INIT_WORK(&mddev->del_work, mddev_delayed_delete); queue_work(md_misc_wq, &mddev->del_work); } spin_unlock(&all_mddevs_lock); } static void md_safemode_timeout(struct timer_list *t); +static void md_start_sync(struct work_struct *ws); void mddev_init(struct mddev *mddev) { @@ -662,6 +662,9 @@ void mddev_init(struct mddev *mddev) mddev->resync_min = 0; mddev->resync_max = MaxSector; mddev->level = LEVEL_NONE; + + INIT_WORK(&mddev->sync_work, md_start_sync); + INIT_WORK(&mddev->del_work, mddev_delayed_delete); } EXPORT_SYMBOL_GPL(mddev_init); @@ -9256,7 +9259,7 @@ no_add: static void md_start_sync(struct work_struct *ws) { - struct mddev *mddev = container_of(ws, struct mddev, del_work); + struct mddev *mddev = container_of(ws, struct mddev, sync_work); rcu_assign_pointer(mddev->sync_thread, md_register_thread(md_do_sync, mddev, "resync")); @@ -9469,8 +9472,7 @@ void md_check_recovery(struct mddev *mddev) */ md_bitmap_write_all(mddev->bitmap); } - INIT_WORK(&mddev->del_work, md_start_sync); - queue_work(md_misc_wq, &mddev->del_work); + queue_work(md_misc_wq, &mddev->sync_work); goto unlock; } not_running: diff --git a/drivers/md/md.h b/drivers/md/md.h index 7c9c13abd7ca..d8c8b8c532bb 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -453,7 +453,10 @@ struct mddev { struct kernfs_node *sysfs_degraded; /*handle for 'degraded' */ struct kernfs_node *sysfs_level; /*handle for 'level' */ - struct work_struct del_work; /* used for delayed sysfs removal */ + /* used for delayed sysfs removal */ + struct work_struct del_work; + /* used for register new sync thread */ + struct work_struct sync_work; /* "lock" protects: * flush_bio transition from NULL to !NULL From 897c62a1cae6485870a45934b22c180016a0570f Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:17 +0800 Subject: [PATCH 02/16] md: factor out a helper to choose sync action from md_check_recovery() There are no functional changes, on the one hand make the code cleaner, on the other hand prevent following checkpatch error in the next patch to delay choosing sync action to md_start_sync(). ERROR: do not use assignment in if condition + } else if ((spares = remove_and_add_spares(mddev, NULL))) { Signed-off-by: Yu Kuai Reviewed-by: Xiao Ni Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-3-yukuai1@huaweicloud.com --- drivers/md/md.c | 70 +++++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index fb9aae4accb0..e6118755fca1 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9257,6 +9257,50 @@ no_add: return spares; } +static bool md_choose_sync_action(struct mddev *mddev, int *spares) +{ + /* Check if reshape is in progress first. */ + if (mddev->reshape_position != MaxSector) { + if (mddev->pers->check_reshape == NULL || + mddev->pers->check_reshape(mddev) != 0) + return false; + + set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); + clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); + return true; + } + + /* + * Remove any failed drives, then add spares if possible. Spares are + * also removed and re-added, to allow the personality to fail the + * re-add. + */ + *spares = remove_and_add_spares(mddev, NULL); + if (*spares) { + clear_bit(MD_RECOVERY_SYNC, &mddev->recovery); + clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); + clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); + + /* Start new recovery. */ + set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); + return true; + } + + /* Check if recovery is in progress. */ + if (mddev->recovery_cp < MaxSector) { + set_bit(MD_RECOVERY_SYNC, &mddev->recovery); + clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); + return true; + } + + /* Delay to choose resync/check/repair in md_do_sync(). */ + if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) + return true; + + /* Nothing to be done */ + return false; +} + static void md_start_sync(struct work_struct *ws) { struct mddev *mddev = container_of(ws, struct mddev, sync_work); @@ -9438,32 +9482,8 @@ void md_check_recovery(struct mddev *mddev) if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) || test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) goto not_running; - /* no recovery is running. - * remove any failed drives, then - * add spares if possible. - * Spares are also removed and re-added, to allow - * the personality to fail the re-add. - */ - - if (mddev->reshape_position != MaxSector) { - if (mddev->pers->check_reshape == NULL || - mddev->pers->check_reshape(mddev) != 0) - /* Cannot proceed */ - goto not_running; - set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); - clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); - } else if ((spares = remove_and_add_spares(mddev, NULL))) { - clear_bit(MD_RECOVERY_SYNC, &mddev->recovery); - clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); - clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); - set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); - } else if (mddev->recovery_cp < MaxSector) { - set_bit(MD_RECOVERY_SYNC, &mddev->recovery); - clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); - } else if (!test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) - /* nothing to be done ... */ + if (!md_choose_sync_action(mddev, &spares)) goto not_running; - if (mddev->pers->sync_request) { if (spares) { /* We are adding a device or devices to an array From db5e653d7c9fae8ed61da58ab5e9a8db2cd61a2b Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:18 +0800 Subject: [PATCH 03/16] md: delay choosing sync action to md_start_sync() Before this patch, for read-write array: 1) md_check_recover() found that something need to be done, and it'll try to grab 'reconfig_mutex'. The case that md_check_recover() need to do something: - array is not suspend; - super_block need to be updated; - 'MD_RECOVERY_NEEDED' or 'MD_RECOVERY_DONE' is set; - unusual case related to safemode; 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set, md_check_recover() will try to choose a sync action, and then queue a work md_start_sync(). 3) md_start_sync() register sync_thread; After this patch, 1) is the same; 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set, queue a work md_start_sync() directly; 3) md_start_sync() will try to choose a sync action, and then register sync_thread(); Because 'MD_RECOVERY_RUNNING' is cleared when sync_thread is done, 2) and 3) and md_do_sync() is always ran in serial and they can never concurrent, this change should not introduce any behavior change for now. Also fix a problem that md_start_sync() can clear 'MD_RECOVERY_RUNNING' without protection in error path, which might affect the logical in md_check_recovery(). The advantage to change this is that array reconfiguration is independent from daemon now, and it'll be much easier to synchronize it with io, consider that io may rely on daemon thread to be done. Signed-off-by: Yu Kuai Reviewed-by: Xiao Ni Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-4-yukuai1@huaweicloud.com --- drivers/md/md.c | 73 ++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index e6118755fca1..52fe795f9011 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9304,6 +9304,22 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares) static void md_start_sync(struct work_struct *ws) { struct mddev *mddev = container_of(ws, struct mddev, sync_work); + int spares = 0; + + mddev_lock_nointr(mddev); + + if (!md_choose_sync_action(mddev, &spares)) + goto not_running; + + if (!mddev->pers->sync_request) + goto not_running; + + /* + * We are adding a device or devices to an array which has the bitmap + * stored on all devices. So make sure all bitmap pages get written. + */ + if (spares) + md_bitmap_write_all(mddev->bitmap); rcu_assign_pointer(mddev->sync_thread, md_register_thread(md_do_sync, mddev, "resync")); @@ -9311,20 +9327,27 @@ static void md_start_sync(struct work_struct *ws) pr_warn("%s: could not start resync thread...\n", mdname(mddev)); /* leave the spares where they are, it shouldn't hurt */ - clear_bit(MD_RECOVERY_SYNC, &mddev->recovery); - clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); - clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); - clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); - clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); - wake_up(&resync_wait); - if (test_and_clear_bit(MD_RECOVERY_RECOVER, - &mddev->recovery)) - if (mddev->sysfs_action) - sysfs_notify_dirent_safe(mddev->sysfs_action); - } else - md_wakeup_thread(mddev->sync_thread); + goto not_running; + } + + mddev_unlock(mddev); + md_wakeup_thread(mddev->sync_thread); sysfs_notify_dirent_safe(mddev->sysfs_action); md_new_event(); + return; + +not_running: + clear_bit(MD_RECOVERY_SYNC, &mddev->recovery); + clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); + clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); + clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); + clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); + mddev_unlock(mddev); + + wake_up(&resync_wait); + if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery) && + mddev->sysfs_action) + sysfs_notify_dirent_safe(mddev->sysfs_action); } /* @@ -9392,7 +9415,6 @@ void md_check_recovery(struct mddev *mddev) return; if (mddev_trylock(mddev)) { - int spares = 0; bool try_set_sync = mddev->safemode != 0; if (!mddev->external && mddev->safemode == 1) @@ -9479,31 +9501,14 @@ void md_check_recovery(struct mddev *mddev) clear_bit(MD_RECOVERY_INTR, &mddev->recovery); clear_bit(MD_RECOVERY_DONE, &mddev->recovery); - if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) || - test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) - goto not_running; - if (!md_choose_sync_action(mddev, &spares)) - goto not_running; - if (mddev->pers->sync_request) { - if (spares) { - /* We are adding a device or devices to an array - * which has the bitmap stored on all devices. - * So make sure all bitmap pages get written - */ - md_bitmap_write_all(mddev->bitmap); - } + if (test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) && + !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) { queue_work(md_misc_wq, &mddev->sync_work); - goto unlock; - } - not_running: - if (!mddev->sync_thread) { + } else { clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); wake_up(&resync_wait); - if (test_and_clear_bit(MD_RECOVERY_RECOVER, - &mddev->recovery)) - if (mddev->sysfs_action) - sysfs_notify_dirent_safe(mddev->sysfs_action); } + unlock: wake_up(&mddev->sb_wait); mddev_unlock(mddev); From 3389d57f97531efe9e285a0740c9b767d6f29f6c Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:19 +0800 Subject: [PATCH 04/16] md: factor out a helper rdev_removeable() from remove_and_add_spares() There are no functional changes, just to make the code simpler and prepare to delay remove_and_add_spares() to md_start_sync(). Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-5-yukuai1@huaweicloud.com --- drivers/md/md.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 52fe795f9011..3118ab26cef6 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9164,6 +9164,42 @@ void md_do_sync(struct md_thread *thread) } EXPORT_SYMBOL_GPL(md_do_sync); +static bool rdev_removeable(struct md_rdev *rdev) +{ + /* rdev is not used. */ + if (rdev->raid_disk < 0) + return false; + + /* There are still inflight io, don't remove this rdev. */ + if (atomic_read(&rdev->nr_pending)) + return false; + + /* + * An error occurred but has not yet been acknowledged by the metadata + * handler, don't remove this rdev. + */ + if (test_bit(Blocked, &rdev->flags)) + return false; + + /* Fautly rdev is not used, it's safe to remove it. */ + if (test_bit(Faulty, &rdev->flags)) + return true; + + /* Journal disk can only be removed if it's faulty. */ + if (test_bit(Journal, &rdev->flags)) + return false; + + /* + * 'In_sync' is cleared while 'raid_disk' is valid, which means + * replacement has just become active from pers->spare_active(), and + * then pers->hot_remove_disk() will replace this rdev with replacement. + */ + if (!test_bit(In_sync, &rdev->flags)) + return true; + + return false; +} + static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this) { @@ -9196,12 +9232,8 @@ static int remove_and_add_spares(struct mddev *mddev, synchronize_rcu(); rdev_for_each(rdev, mddev) { if ((this == NULL || rdev == this) && - rdev->raid_disk >= 0 && - !test_bit(Blocked, &rdev->flags) && - ((test_bit(RemoveSynchronized, &rdev->flags) || - (!test_bit(In_sync, &rdev->flags) && - !test_bit(Journal, &rdev->flags))) && - atomic_read(&rdev->nr_pending)==0)) { + (test_bit(RemoveSynchronized, &rdev->flags) || + rdev_removeable(rdev))) { if (mddev->pers->hot_remove_disk( mddev, rdev) == 0) { sysfs_unlink_rdev(mddev, rdev); From b172a0704d0ddea3a55f3633f8249c3ce7d960bc Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:20 +0800 Subject: [PATCH 05/16] md: factor out a helper rdev_is_spare() from remove_and_add_spares() There are no functional changes, just to make the code simpler and prepare to delay remove_and_add_spares() to md_start_sync(). Signed-off-by: Yu Kuai Reviewed-by: Xiao Ni Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-6-yukuai1@huaweicloud.com --- 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 3118ab26cef6..fd96729c6655 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9200,6 +9200,14 @@ static bool rdev_removeable(struct md_rdev *rdev) return false; } +static bool rdev_is_spare(struct md_rdev *rdev) +{ + return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 && + !test_bit(In_sync, &rdev->flags) && + !test_bit(Journal, &rdev->flags) && + !test_bit(Faulty, &rdev->flags); +} + static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this) { @@ -9255,13 +9263,10 @@ static int remove_and_add_spares(struct mddev *mddev, rdev_for_each(rdev, mddev) { if (this && this != rdev) continue; + if (rdev_is_spare(rdev)) + spares++; if (test_bit(Candidate, &rdev->flags)) continue; - if (rdev->raid_disk >= 0 && - !test_bit(In_sync, &rdev->flags) && - !test_bit(Journal, &rdev->flags) && - !test_bit(Faulty, &rdev->flags)) - spares++; if (rdev->raid_disk >= 0) continue; if (test_bit(Faulty, &rdev->flags)) From a0ae7e4e0bc00ae178e42391157e8ddb88b2838a Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:21 +0800 Subject: [PATCH 06/16] md: factor out a helper rdev_addable() from remove_and_add_spares() There are no functional changes, just to make the code simpler and prepare to delay remove_and_add_spares() to md_start_sync(). Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-7-yukuai1@huaweicloud.com --- drivers/md/md.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index fd96729c6655..324e1f3243b6 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9208,6 +9208,31 @@ static bool rdev_is_spare(struct md_rdev *rdev) !test_bit(Faulty, &rdev->flags); } +static bool rdev_addable(struct md_rdev *rdev) +{ + /* rdev is already used, don't add it again. */ + if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 || + test_bit(Faulty, &rdev->flags)) + return false; + + /* Allow to add journal disk. */ + if (test_bit(Journal, &rdev->flags)) + return true; + + /* Allow to add if array is read-write. */ + if (md_is_rdwr(rdev->mddev)) + return true; + + /* + * For read-only array, only allow to readd a rdev. And if bitmap is + * used, don't allow to readd a rdev that is too old. + */ + if (rdev->saved_raid_disk >= 0 && !test_bit(Bitmap_sync, &rdev->flags)) + return true; + + return false; +} + static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this) { @@ -9265,20 +9290,10 @@ static int remove_and_add_spares(struct mddev *mddev, continue; if (rdev_is_spare(rdev)) spares++; - if (test_bit(Candidate, &rdev->flags)) + if (!rdev_addable(rdev)) continue; - if (rdev->raid_disk >= 0) - continue; - if (test_bit(Faulty, &rdev->flags)) - continue; - if (!test_bit(Journal, &rdev->flags)) { - if (!md_is_rdwr(mddev) && - !(rdev->saved_raid_disk >= 0 && - !test_bit(Bitmap_sync, &rdev->flags))) - continue; - + if (!test_bit(Journal, &rdev->flags)) rdev->recovery_offset = 0; - } if (mddev->pers->hot_add_disk(mddev, rdev) == 0) { /* failure here is OK */ sysfs_link_rdev(mddev, rdev); From 81e2ce1b3d5a896e24fe5af83896fec057860a25 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:16:22 +0800 Subject: [PATCH 07/16] md: delay remove_and_add_spares() for read only array to md_start_sync() Before this patch, for read-only array: md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will call remove_and_add_spares() directly to try to remove and add rdevs from array. After this patch: 1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the worker 'sync_work' is not pending, and there are rdevs can be added or removed, then it will queue new work md_start_sync(); 2) md_start_sync() will call remove_and_add_spares() and exist; This change make sure that array reconfiguration is independent from daemon, and it'll be much easier to synchronize it with io, consier that io may rely on daemon thread to be done. Also fix a problem that 'pers->spars_active' is called after remove_and_add_spares(), which order is wrong, because spares must active first, and then remove_and_add_spares() can add spares to the array, like what read-write case does: 1) daemon set 'MD_RECOVERY_RUNNING', register new sync thread to do recovery; 2) recovery is done, md_do_sync() set 'MD_RECOVERY_DONE' before return; 3) daemon call 'pers->spars_active', and clear 'MD_RECOVERY_RUNNING'; 4) in the next round of daemon, call remove_and_add_spares() to add spares to the array. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825031622.1530464-8-yukuai1@huaweicloud.com --- drivers/md/md.c | 61 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 324e1f3243b6..94abc6b7edbd 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -4875,6 +4875,7 @@ action_store(struct mddev *mddev, const char *page, size_t len) /* A write to sync_action is enough to justify * canceling read-auto mode */ + flush_work(&mddev->sync_work); mddev->ro = MD_RDWR; md_wakeup_thread(mddev->sync_thread); } @@ -7649,6 +7650,10 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode, mutex_unlock(&mddev->open_mutex); sync_blockdev(bdev); } + + if (!md_is_rdwr(mddev)) + flush_work(&mddev->sync_work); + err = mddev_lock(mddev); if (err) { pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n", @@ -8573,6 +8578,7 @@ bool md_write_start(struct mddev *mddev, struct bio *bi) BUG_ON(mddev->ro == MD_RDONLY); if (mddev->ro == MD_AUTO_READ) { /* need to switch to read/write */ + flush_work(&mddev->sync_work); mddev->ro = MD_RDWR; set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); md_wakeup_thread(mddev->thread); @@ -9233,6 +9239,16 @@ static bool rdev_addable(struct md_rdev *rdev) return false; } +static bool md_spares_need_change(struct mddev *mddev) +{ + struct md_rdev *rdev; + + rdev_for_each(rdev, mddev) + if (rdev_removeable(rdev) || rdev_addable(rdev)) + return true; + return false; +} + static int remove_and_add_spares(struct mddev *mddev, struct md_rdev *this) { @@ -9360,6 +9376,18 @@ static void md_start_sync(struct work_struct *ws) mddev_lock_nointr(mddev); + if (!md_is_rdwr(mddev)) { + /* + * On a read-only array we can: + * - remove failed devices + * - add already-in_sync devices if the array itself is in-sync. + * As we only add devices that are already in-sync, we can + * activate the spares immediately. + */ + remove_and_add_spares(mddev, NULL); + goto not_running; + } + if (!md_choose_sync_action(mddev, &spares)) goto not_running; @@ -9474,30 +9502,43 @@ void md_check_recovery(struct mddev *mddev) if (!md_is_rdwr(mddev)) { struct md_rdev *rdev; + + if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) { + /* sync_work already queued. */ + clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery); + goto unlock; + } + if (!mddev->external && mddev->in_sync) - /* 'Blocked' flag not needed as failed devices + /* + * 'Blocked' flag not needed as failed devices * will be recorded if array switched to read/write. * Leaving it set will prevent the device * from being removed. */ rdev_for_each(rdev, mddev) clear_bit(Blocked, &rdev->flags); - /* On a read-only array we can: - * - remove failed devices - * - add already-in_sync devices if the array itself - * is in-sync. - * As we only add devices that are already in-sync, - * we can activate the spares immediately. - */ - remove_and_add_spares(mddev, NULL); - /* There is no thread, but we need to call + + /* + * There is no thread, but we need to call * ->spare_active and clear saved_raid_disk */ set_bit(MD_RECOVERY_INTR, &mddev->recovery); md_reap_sync_thread(mddev); + + /* + * Let md_start_sync() to remove and add rdevs to the + * array. + */ + if (md_spares_need_change(mddev)) { + set_bit(MD_RECOVERY_RUNNING, &mddev->recovery); + queue_work(md_misc_wq, &mddev->sync_work); + } + clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery); clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags); + goto unlock; } From d58eff83bd3c6166944f6b159544438385d48549 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:50 +0800 Subject: [PATCH 08/16] md: initialize 'active_io' while allocating mddev 'active_io' is used for mddev_suspend() and it's initialized in md_run(), this restrict that 'reconfig_mutex' must be held and "mddev->pers" must be set before calling mddev_suspend(). Initialize 'active_io' early so that mddev_suspend() is safe to call once mddev is allocated, this will be helpful to refactor mddev_suspend() in following patches. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-2-yukuai1@huaweicloud.com --- drivers/md/dm-raid.c | 7 +++++- drivers/md/md.c | 53 +++++++++++++++++++++++++++----------------- drivers/md/md.h | 3 ++- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 5f9991765f27..69805d37e113 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -749,7 +749,11 @@ static struct raid_set *raid_set_alloc(struct dm_target *ti, struct raid_type *r return ERR_PTR(-ENOMEM); } - mddev_init(&rs->md); + if (mddev_init(&rs->md)) { + kfree(rs); + ti->error = "Cannot initialize raid context"; + return ERR_PTR(-ENOMEM); + } rs->raid_disks = raid_devs; rs->delta_disks = 0; @@ -798,6 +802,7 @@ static void raid_set_free(struct raid_set *rs) dm_put_device(rs->ti, rs->dev[i].data_dev); } + mddev_destroy(&rs->md); kfree(rs); } diff --git a/drivers/md/md.c b/drivers/md/md.c index 94abc6b7edbd..0f642785073f 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -639,8 +639,20 @@ void mddev_put(struct mddev *mddev) static void md_safemode_timeout(struct timer_list *t); static void md_start_sync(struct work_struct *ws); -void mddev_init(struct mddev *mddev) +static void active_io_release(struct percpu_ref *ref) { + struct mddev *mddev = container_of(ref, struct mddev, active_io); + + wake_up(&mddev->sb_wait); +} + +int mddev_init(struct mddev *mddev) +{ + + if (percpu_ref_init(&mddev->active_io, active_io_release, + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) + return -ENOMEM; + mutex_init(&mddev->open_mutex); mutex_init(&mddev->reconfig_mutex); mutex_init(&mddev->sync_mutex); @@ -665,9 +677,17 @@ void mddev_init(struct mddev *mddev) INIT_WORK(&mddev->sync_work, md_start_sync); INIT_WORK(&mddev->del_work, mddev_delayed_delete); + + return 0; } EXPORT_SYMBOL_GPL(mddev_init); +void mddev_destroy(struct mddev *mddev) +{ + percpu_ref_exit(&mddev->active_io); +} +EXPORT_SYMBOL_GPL(mddev_destroy); + static struct mddev *mddev_find_locked(dev_t unit) { struct mddev *mddev; @@ -711,13 +731,16 @@ static struct mddev *mddev_alloc(dev_t unit) new = kzalloc(sizeof(*new), GFP_KERNEL); if (!new) return ERR_PTR(-ENOMEM); - mddev_init(new); + + error = mddev_init(new); + if (error) + goto out_free_new; spin_lock(&all_mddevs_lock); if (unit) { error = -EEXIST; if (mddev_find_locked(unit)) - goto out_free_new; + goto out_destroy_new; new->unit = unit; if (MAJOR(unit) == MD_MAJOR) new->md_minor = MINOR(unit); @@ -728,7 +751,7 @@ static struct mddev *mddev_alloc(dev_t unit) error = -ENODEV; new->unit = mddev_alloc_unit(); if (!new->unit) - goto out_free_new; + goto out_destroy_new; new->md_minor = MINOR(new->unit); new->hold_active = UNTIL_STOP; } @@ -736,8 +759,11 @@ static struct mddev *mddev_alloc(dev_t unit) list_add(&new->all_mddevs, &all_mddevs); spin_unlock(&all_mddevs_lock); return new; -out_free_new: + +out_destroy_new: spin_unlock(&all_mddevs_lock); + mddev_destroy(new); +out_free_new: kfree(new); return ERR_PTR(error); } @@ -748,6 +774,7 @@ static void mddev_free(struct mddev *mddev) list_del(&mddev->all_mddevs); spin_unlock(&all_mddevs_lock); + mddev_destroy(mddev); kfree(mddev); } @@ -5787,12 +5814,6 @@ static void md_safemode_timeout(struct timer_list *t) } static int start_dirty_degraded; -static void active_io_release(struct percpu_ref *ref) -{ - struct mddev *mddev = container_of(ref, struct mddev, active_io); - - wake_up(&mddev->sb_wait); -} int md_run(struct mddev *mddev) { @@ -5873,15 +5894,10 @@ int md_run(struct mddev *mddev) nowait = nowait && bdev_nowait(rdev->bdev); } - err = percpu_ref_init(&mddev->active_io, active_io_release, - PERCPU_REF_ALLOW_REINIT, GFP_KERNEL); - if (err) - return err; - if (!bioset_initialized(&mddev->bio_set)) { err = bioset_init(&mddev->bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS); if (err) - goto exit_active_io; + return err; } if (!bioset_initialized(&mddev->sync_set)) { err = bioset_init(&mddev->sync_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS); @@ -6078,8 +6094,6 @@ exit_sync_set: bioset_exit(&mddev->sync_set); exit_bio_set: bioset_exit(&mddev->bio_set); -exit_active_io: - percpu_ref_exit(&mddev->active_io); return err; } EXPORT_SYMBOL_GPL(md_run); @@ -6295,7 +6309,6 @@ static void __md_stop(struct mddev *mddev) module_put(pers->owner); clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery); - percpu_ref_exit(&mddev->active_io); bioset_exit(&mddev->bio_set); bioset_exit(&mddev->sync_set); bioset_exit(&mddev->io_clone_set); diff --git a/drivers/md/md.h b/drivers/md/md.h index d8c8b8c532bb..3344c47f8544 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -798,7 +798,8 @@ extern int md_integrity_register(struct mddev *mddev); extern int md_integrity_add_rdev(struct md_rdev *rdev, struct mddev *mddev); extern int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale); -extern void mddev_init(struct mddev *mddev); +extern int mddev_init(struct mddev *mddev); +extern void mddev_destroy(struct mddev *mddev); struct mddev *md_alloc(dev_t dev, char *name); void mddev_put(struct mddev *mddev); extern int md_run(struct mddev *mddev); From b8494823e236326500aa1004155e83f748dd10da Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:51 +0800 Subject: [PATCH 09/16] md: initialize 'writes_pending' while allocating mddev Currently 'writes_pending' is initialized in pers->run for raid1/5/10, and it's freed while deleing mddev, instead of pers->free. pers->run can be called multiple times before mddev is deleted, and a helper mddev_init_writes_pending() is used to prevent 'writes_pending' to be initialized multiple times, this usage is safe but a litter weird. On the other hand, 'writes_pending' is only initialized for raid1/5/10, however, it's used in common layer, for example: array_state_store set_in_sync if (!mddev->in_sync) -> in_sync is used for all levels // access writes_pending There might be some implicit dependency that I don't recognized to make sure 'writes_pending' can only be accessed for raid1/5/10, but there are no comments about that. By the way, it make sense to initialize 'writes_pending' in common layer because there are already three levels use it. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-3-yukuai1@huaweicloud.com --- drivers/md/md.c | 29 ++++++++++++----------------- drivers/md/md.h | 1 - drivers/md/raid1.c | 3 +-- drivers/md/raid10.c | 3 --- drivers/md/raid5.c | 3 --- 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 0f642785073f..0212f504f36a 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -646,6 +646,8 @@ static void active_io_release(struct percpu_ref *ref) wake_up(&mddev->sb_wait); } +static void no_op(struct percpu_ref *r) {} + int mddev_init(struct mddev *mddev) { @@ -653,6 +655,15 @@ int mddev_init(struct mddev *mddev) PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) return -ENOMEM; + if (percpu_ref_init(&mddev->writes_pending, no_op, + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) { + percpu_ref_exit(&mddev->active_io); + return -ENOMEM; + } + + /* We want to start with the refcount at zero */ + percpu_ref_put(&mddev->writes_pending); + mutex_init(&mddev->open_mutex); mutex_init(&mddev->reconfig_mutex); mutex_init(&mddev->sync_mutex); @@ -685,6 +696,7 @@ EXPORT_SYMBOL_GPL(mddev_init); void mddev_destroy(struct mddev *mddev) { percpu_ref_exit(&mddev->active_io); + percpu_ref_exit(&mddev->writes_pending); } EXPORT_SYMBOL_GPL(mddev_destroy); @@ -5628,21 +5640,6 @@ static void mddev_delayed_delete(struct work_struct *ws) kobject_put(&mddev->kobj); } -static void no_op(struct percpu_ref *r) {} - -int mddev_init_writes_pending(struct mddev *mddev) -{ - if (mddev->writes_pending.percpu_count_ptr) - return 0; - if (percpu_ref_init(&mddev->writes_pending, no_op, - PERCPU_REF_ALLOW_REINIT, GFP_KERNEL) < 0) - return -ENOMEM; - /* We want to start with the refcount at zero */ - percpu_ref_put(&mddev->writes_pending); - return 0; -} -EXPORT_SYMBOL_GPL(mddev_init_writes_pending); - struct mddev *md_alloc(dev_t dev, char *name) { /* @@ -6323,7 +6320,6 @@ void md_stop(struct mddev *mddev) */ __md_stop_writes(mddev); __md_stop(mddev); - percpu_ref_exit(&mddev->writes_pending); } EXPORT_SYMBOL_GPL(md_stop); @@ -7907,7 +7903,6 @@ static void md_free_disk(struct gendisk *disk) { struct mddev *mddev = disk->private_data; - percpu_ref_exit(&mddev->writes_pending); mddev_free(mddev); } diff --git a/drivers/md/md.h b/drivers/md/md.h index 3344c47f8544..b628c292506e 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -771,7 +771,6 @@ extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t 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 int mddev_init_writes_pending(struct mddev *mddev); extern bool md_write_start(struct mddev *mddev, struct bio *bi); extern void md_write_inc(struct mddev *mddev, struct bio *bi); extern void md_write_end(struct mddev *mddev); diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 2aabac773fe7..3a78f79ee6d5 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -3122,8 +3122,7 @@ static int raid1_run(struct mddev *mddev) mdname(mddev)); return -EIO; } - if (mddev_init_writes_pending(mddev) < 0) - return -ENOMEM; + /* * copy the already verified devices into our private RAID1 * bookkeeping area. [whatever we allocate in run(), diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 023413120851..a5927e98dc67 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -4154,9 +4154,6 @@ static int raid10_run(struct mddev *mddev) sector_t min_offset_diff = 0; int first = 1; - if (mddev_init_writes_pending(mddev) < 0) - return -ENOMEM; - if (mddev->private == NULL) { conf = setup_conf(mddev); if (IS_ERR(conf)) diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 4cb9c608ee19..6383723468e5 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -7778,9 +7778,6 @@ static int raid5_run(struct mddev *mddev) long long min_offset_diff = 0; int first = 1; - if (mddev_init_writes_pending(mddev) < 0) - return -ENOMEM; - if (mddev->recovery_cp != MaxSector) pr_notice("md/raid:%s: not clean -- starting background reconstruction\n", mdname(mddev)); From b721e7885eb242aa2459ee66bb42ceef1bcf0f0c Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:52 +0800 Subject: [PATCH 10/16] md: don't rely on 'mddev->pers' to be set in mddev_suspend() 'active_io' used to be initialized while the array is running, and 'mddev->pers' is set while the array is running as well. Hence caller must hold 'reconfig_mutex' and guarantee 'mddev->pers' is set before calling mddev_suspend(). Now that 'active_io' is initialized when mddev is allocated, such restriction doesn't exist anymore. In the meantime, follow up patches will refactor mddev_suspend(), hence add checking for 'mddev->pers' to prevent null-ptr-deref. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-4-yukuai1@huaweicloud.com --- drivers/md/md.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 0212f504f36a..889c282a91ea 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -449,7 +449,7 @@ void mddev_suspend(struct mddev *mddev) set_bit(MD_ALLOW_SB_UPDATE, &mddev->flags); percpu_ref_kill(&mddev->active_io); - if (mddev->pers->prepare_suspend) + if (mddev->pers && mddev->pers->prepare_suspend) mddev->pers->prepare_suspend(mddev); wait_event(mddev->sb_wait, percpu_ref_is_zero(&mddev->active_io)); From b71fe4ac7531d67e6fc8c287cbcb2b176aa93833 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:53 +0800 Subject: [PATCH 11/16] md-bitmap: remove the checking of 'pers->quiesce' from location_store() After commit 4d27e927344a ("md: don't quiesce in mddev_suspend()"), there is no need to check 'pers->quiesce' before calling mddev_suspend(). Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-5-yukuai1@huaweicloud.com --- drivers/md/md-bitmap.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index 6f9ff14971f9..f38c7f3156cb 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -2352,10 +2352,6 @@ location_store(struct mddev *mddev, const char *buf, size_t len) if (rv) return rv; if (mddev->pers) { - if (!mddev->pers->quiesce) { - rv = -EBUSY; - goto out; - } if (mddev->recovery || mddev->sync_thread) { rv = -EBUSY; goto out; From 158d32af8710777b146f5303a5ca066b493d18e8 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:54 +0800 Subject: [PATCH 12/16] md-bitmap: suspend array earlier in location_store() Now that mddev_suspend() doean't rely on 'mddev->pers' to be set, it's safe to call mddev_suspend() earlier. This will also be helper to refactor mddev_suspend() later. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-6-yukuai1@huaweicloud.com --- drivers/md/md-bitmap.c | 43 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index f38c7f3156cb..0c661e5036bb 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -2351,6 +2351,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len) rv = mddev_lock(mddev); if (rv) return rv; + + mddev_suspend(mddev); if (mddev->pers) { if (mddev->recovery || mddev->sync_thread) { rv = -EBUSY; @@ -2365,11 +2367,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len) rv = -EBUSY; goto out; } - if (mddev->pers) { - mddev_suspend(mddev); - md_bitmap_destroy(mddev); - mddev_resume(mddev); - } + + md_bitmap_destroy(mddev); mddev->bitmap_info.offset = 0; if (mddev->bitmap_info.file) { struct file *f = mddev->bitmap_info.file; @@ -2379,6 +2378,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len) } else { /* No bitmap, OK to set a location */ long long offset; + struct bitmap *bitmap; + if (strncmp(buf, "none", 4) == 0) /* nothing to be done */; else if (strncmp(buf, "file:", 5) == 0) { @@ -2402,25 +2403,20 @@ location_store(struct mddev *mddev, const char *buf, size_t len) rv = -EINVAL; goto out; } + mddev->bitmap_info.offset = offset; - if (mddev->pers) { - struct bitmap *bitmap; - bitmap = md_bitmap_create(mddev, -1); - mddev_suspend(mddev); - if (IS_ERR(bitmap)) - rv = PTR_ERR(bitmap); - else { - mddev->bitmap = bitmap; - rv = md_bitmap_load(mddev); - if (rv) - mddev->bitmap_info.offset = 0; - } - if (rv) { - md_bitmap_destroy(mddev); - mddev_resume(mddev); - goto out; - } - mddev_resume(mddev); + bitmap = md_bitmap_create(mddev, -1); + if (IS_ERR(bitmap)) { + rv = PTR_ERR(bitmap); + goto out; + } + + mddev->bitmap = bitmap; + rv = md_bitmap_load(mddev); + if (rv) { + mddev->bitmap_info.offset = 0; + md_bitmap_destroy(mddev); + goto out; } } } @@ -2433,6 +2429,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len) } rv = 0; out: + mddev_resume(mddev); mddev_unlock(mddev); if (rv) return rv; From a2a9f16838509475ea6801f7794a89e03d55e3ed Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:55 +0800 Subject: [PATCH 13/16] md: don't check 'mddev->pers' from suspend_hi_store() Now that mddev_suspend() doean't rely on 'mddev->pers' to be set, it's safe to remove such checking. This will also allow the array to be suspended even before the array is ran. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-7-yukuai1@huaweicloud.com --- drivers/md/md.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 889c282a91ea..081b6ec2da52 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5226,18 +5226,13 @@ suspend_hi_store(struct mddev *mddev, const char *buf, size_t len) err = mddev_lock(mddev); if (err) return err; - err = -EINVAL; - if (mddev->pers == NULL) - goto unlock; mddev_suspend(mddev); mddev->suspend_hi = new; mddev_resume(mddev); - err = 0; -unlock: mddev_unlock(mddev); - return err ?: len; + return len; } static struct md_sysfs_entry md_suspend_hi = __ATTR(suspend_hi, S_IRUGO|S_IWUSR, suspend_hi_show, suspend_hi_store); From 54d21eb6ad5e57e70157590397ba01b9faed6b59 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Fri, 25 Aug 2023 11:09:56 +0800 Subject: [PATCH 14/16] md: don't check 'mddev->pers' and 'pers->quiesce' from suspend_lo_store() Now that mddev_suspend() doean't rely on 'mddev->pers' to be set, it's safe to remove such checking. This will also allow the array to be suspended even before the array is ran. Signed-off-by: Yu Kuai Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230825030956.1527023-8-yukuai1@huaweicloud.com --- drivers/md/md.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 081b6ec2da52..10cb4dfbf4ae 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5189,18 +5189,13 @@ suspend_lo_store(struct mddev *mddev, const char *buf, size_t len) err = mddev_lock(mddev); if (err) return err; - err = -EINVAL; - if (mddev->pers == NULL || - mddev->pers->quiesce == NULL) - goto unlock; + mddev_suspend(mddev); mddev->suspend_lo = new; mddev_resume(mddev); - err = 0; -unlock: mddev_unlock(mddev); - return err ?: len; + return len; } static struct md_sysfs_entry md_suspend_lo = __ATTR(suspend_lo, S_IRUGO|S_IWUSR, suspend_lo_show, suspend_lo_store); From e887544d7620f1d3cef017e45df7bc625182caff Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:03:28 -0700 Subject: [PATCH 15/16] md/md-linear: Annotate struct linear_conf with __counted_by Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct linear_conf. Additionally, since the element count member must be set before accessing the annotated flexible array member, move its initialization earlier. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Song Liu Cc: linux-raid@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: Gustavo A. R. Silva Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230915200328.never.064-kees@kernel.org --- drivers/md/md-linear.c | 26 +++++++++++++------------- drivers/md/md-linear.h | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c index 71ac99646827..ae2826e9645b 100644 --- a/drivers/md/md-linear.c +++ b/drivers/md/md-linear.c @@ -69,6 +69,19 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks) if (!conf) return NULL; + /* + * conf->raid_disks is copy of mddev->raid_disks. The reason to + * keep a copy of mddev->raid_disks in struct linear_conf is, + * mddev->raid_disks may not be consistent with pointers number of + * conf->disks[] when it is updated in linear_add() and used to + * iterate old conf->disks[] earray in linear_congested(). + * Here conf->raid_disks is always consitent with number of + * pointers in conf->disks[] array, and mddev->private is updated + * with rcu_assign_pointer() in linear_addr(), such race can be + * avoided. + */ + conf->raid_disks = raid_disks; + cnt = 0; conf->array_sectors = 0; @@ -112,19 +125,6 @@ static struct linear_conf *linear_conf(struct mddev *mddev, int raid_disks) conf->disks[i-1].end_sector + conf->disks[i].rdev->sectors; - /* - * conf->raid_disks is copy of mddev->raid_disks. The reason to - * keep a copy of mddev->raid_disks in struct linear_conf is, - * mddev->raid_disks may not be consistent with pointers number of - * conf->disks[] when it is updated in linear_add() and used to - * iterate old conf->disks[] earray in linear_congested(). - * Here conf->raid_disks is always consitent with number of - * pointers in conf->disks[] array, and mddev->private is updated - * with rcu_assign_pointer() in linear_addr(), such race can be - * avoided. - */ - conf->raid_disks = raid_disks; - return conf; out: diff --git a/drivers/md/md-linear.h b/drivers/md/md-linear.h index 24e97db50ebb..5587eeedb882 100644 --- a/drivers/md/md-linear.h +++ b/drivers/md/md-linear.h @@ -12,6 +12,6 @@ struct linear_conf struct rcu_head rcu; sector_t array_sectors; int raid_disks; /* a copy of mddev->raid_disks */ - struct dev_info disks[]; + struct dev_info disks[] __counted_by(raid_disks); }; #endif From ceb0416383988dbd5decd6a70141a3507732c160 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 25 Sep 2023 09:49:17 +0000 Subject: [PATCH 16/16] md: replace deprecated strncpy with memcpy `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. There are three such strncpy uses that this patch addresses: The respective destination buffers are: 1) mddev->clevel 2) clevel 3) mddev->metadata_type We expect mddev->clevel to be NUL-terminated due to its use with format strings: | ret = sprintf(page, "%s\n", mddev->clevel); Furthermore, we can see that mddev->clevel is not expected to be NUL-padded as `md_clean()` merely set's its first byte to NULL -- not the entire buffer: | static void md_clean(struct mddev *mddev) | { | mddev->array_sectors = 0; | mddev->external_size = 0; | ... | mddev->level = LEVEL_NONE; | mddev->clevel[0] = 0; | ... A suitable replacement for this instance is `memcpy` as we know the number of bytes to copy and perform manual NUL-termination at a specified offset. This really decays to just a byte copy from one buffer to another. `strscpy` is also a considerable replacement but using `slen` as the length argument would result in truncation of the last byte unless something like `slen + 1` was provided which isn't the most idiomatic strscpy usage. For the next case, the destination buffer `clevel` is expected to be NUL-terminated based on its usage within kstrtol() which expects NUL-terminated strings. Note that, in context, this code removes a trailing newline which is seemingly not required as kstrtol() can handle trailing newlines implicitly. However, there exists further usage of clevel (or buf) that would also like to have the newline removed. All in all, with similar reasoning to the first case, let's just use memcpy as this is just a byte copy and NUL-termination is handled manually. The third and final case concerning `mddev->metadata_type` is more or less the same as the other two. We expect that it be NUL-terminated based on its usage with seq_printf: | seq_printf(seq, " super external:%s", | mddev->metadata_type); ... and we can surmise that NUL-padding isn't required either due to how it is handled in md_clean(): | static void md_clean(struct mddev *mddev) | { | ... | mddev->metadata_type[0] = 0; | ... So really, all these instances have precisely calculated lengths and purposeful NUL-termination so we can just use memcpy to remove ambiguity surrounding strncpy. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Signed-off-by: Song Liu Link: https://lore.kernel.org/r/20230925-strncpy-drivers-md-md-c-v1-1-2b0093b89c2b@google.com --- drivers/md/md.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 10cb4dfbf4ae..76e2cf609883 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -3921,7 +3921,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) return rv; if (mddev->pers == NULL) { - strncpy(mddev->clevel, buf, slen); + memcpy(mddev->clevel, buf, slen); if (mddev->clevel[slen-1] == '\n') slen--; mddev->clevel[slen] = 0; @@ -3954,7 +3954,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) } /* Now find the new personality */ - strncpy(clevel, buf, slen); + memcpy(clevel, buf, slen); if (clevel[slen-1] == '\n') slen--; clevel[slen] = 0; @@ -4740,7 +4740,7 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len) size_t namelen = len-9; if (namelen >= sizeof(mddev->metadata_type)) namelen = sizeof(mddev->metadata_type)-1; - strncpy(mddev->metadata_type, buf+9, namelen); + memcpy(mddev->metadata_type, buf+9, namelen); mddev->metadata_type[namelen] = 0; if (namelen && mddev->metadata_type[namelen-1] == '\n') mddev->metadata_type[--namelen] = 0;