Hi, 在 2025/05/15 17:08, Xiao Ni 写道:
Now del_gendisk and put_disk are called asynchronously in workqueue work. The asynchronous way also has a problem that the device node can still exist after mdadm --stop command returns in a short window. So udev rule can open this device node and create the struct mddev in kernel again. So put del_gendisk in control path and still leave put_disk in md_kobj_release to avoid uaf. But there is a window that sysfs can be accessed between mddev_unlock and del_gendisk. So some actions (add disk, change level, .e.g) can happen which lead unexpected results. And if we delete MD_DELETED and only use MD_CLOSING in stop control path, the sysfs files can't be accessed if do_md_stop stuck when io hange. So we keep MD_DELETED here and set MD_DELETED before mddev_unlock. Signed-off-by: Xiao Ni <xni@xxxxxxxxxx> --- drivers/md/md.c | 53 ++++++++++++++++++++++++++++++++++++++++++------- drivers/md/md.h | 16 ++++++++++++++- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 9b9950ed6ee9..a62867f34aa8 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -606,15 +606,13 @@ static inline struct mddev *mddev_get(struct mddev *mddev) }static void mddev_delayed_delete(struct work_struct *ws);+static bool can_delete_gendisk(struct mddev *mddev);
Plese define new helper here directly.
static void __mddev_put(struct mddev *mddev){ - if (mddev->raid_disks || !list_empty(&mddev->disks) || - mddev->ctime || mddev->hold_active) - return;- /* Array is not configured at all, and not held active, so destroy it */- set_bit(MD_DELETED, &mddev->flags); + if (can_delete_gendisk(mddev) == false) + return;/** Call queue_work inside the spinlock so that flush_workqueue() after @@ -4400,6 +4398,7 @@ array_state_show(struct mddev *mddev, char *page) return sprintf(page, "%s\n", array_states[st]); }+static void delete_gendisk(struct mddev *mddev);
This helper looks unnecessary, it doesn't make code cleaner.
static int do_md_stop(struct mddev *mddev, int ro); static int md_set_readonly(struct mddev *mddev); static int restart_array(struct mddev *mddev); @@ -4533,6 +4532,9 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len) (err && st == clear)) clear_bit(MD_CLOSING, &mddev->flags);+ if ((st == clear || st == inactive) && !err)+ delete_gendisk(mddev);
I think it's better to do this in mddev_unlock() if MD_DELETED is set, like what we did for rdev. Meanwhile, mddev->to_remove and all related hacks can be removed now.
+ return err ?: len; } static struct md_sysfs_entry md_array_state = @@ -5721,19 +5723,30 @@ md_attr_store(struct kobject *kobj, struct attribute *attr, struct md_sysfs_entry *entry = container_of(attr, struct md_sysfs_entry, attr); struct mddev *mddev = container_of(kobj, struct mddev, kobj); ssize_t rv; + struct kernfs_node *kn = NULL;if (!entry->store)return -EIO; if (!capable(CAP_SYS_ADMIN)) return -EACCES; + + if (entry->store == array_state_store && cmd_match(page, "clear")) + kn = sysfs_break_active_protection(kobj, attr); + spin_lock(&all_mddevs_lock); if (!mddev_get(mddev)) { spin_unlock(&all_mddevs_lock); + if (kn) + sysfs_unbreak_active_protection(kn); return -EBUSY; } spin_unlock(&all_mddevs_lock); rv = entry->store(mddev, page, length); mddev_put(mddev); + + if (kn) + sysfs_unbreak_active_protection(kn); + return rv; }@@ -5746,7 +5759,6 @@ static void md_kobj_release(struct kobject *ko)if (mddev->sysfs_level) sysfs_put(mddev->sysfs_level);- del_gendisk(mddev->gendisk);put_disk(mddev->gendisk); }@@ -6526,6 +6538,28 @@ static int md_set_readonly(struct mddev *mddev)return err; }+static bool can_delete_gendisk(struct mddev *mddev)+{ + if (mddev->raid_disks || !list_empty(&mddev->disks) || + mddev->ctime || mddev->hold_active) + return false; + + return true; +} + +/* Call this function after do_md_stop with mode 0. + * And it can't call this function under reconfig_mutex to + * avoid deadlock(e.g. call del_gendisk under the lock and + * an access to sysfs files waits the lock) + */ +static void delete_gendisk(struct mddev *mddev) +{ + if (can_delete_gendisk(mddev) == false) + return; + + del_gendisk(mddev->gendisk); +} + /* mode: * 0 - completely stop and dis-assemble array * 2 - stop but do not disassemble array @@ -6588,8 +6622,8 @@ static int do_md_stop(struct mddev *mddev, int mode) mddev->bitmap_info.offset = 0;export_array(mddev);- md_clean(mddev); + set_bit(MD_DELETED, &mddev->flags); } md_new_event(); sysfs_notify_dirent_safe(mddev->sysfs_state); @@ -6616,6 +6650,7 @@ static void autorun_array(struct mddev *mddev) if (err) { pr_warn("md: do_md_run() returned %d\n", err); do_md_stop(mddev, 0); + delete_gendisk(mddev); } }@@ -7886,6 +7921,10 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,out: if (cmd == STOP_ARRAY_RO || (err && cmd == STOP_ARRAY)) clear_bit(MD_CLOSING, &mddev->flags); + + if (cmd == STOP_ARRAY && err == 0) + delete_gendisk(mddev); + return err; } #ifdef CONFIG_COMPAT diff --git a/drivers/md/md.h b/drivers/md/md.h index 1cf00a04bcdd..45f1027986e4 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -697,11 +697,25 @@ static inline bool reshape_interrupted(struct mddev *mddev)static inline int __must_check mddev_lock(struct mddev *mddev){ - return mutex_lock_interruptible(&mddev->reconfig_mutex); + int ret = 0; + + ret = mutex_lock_interruptible(&mddev->reconfig_mutex); + + /* MD_DELETED is set in do_md_stop with reconfig_mutex + * So check it here also. + */ + if (!ret && test_bit(MD_DELETED, &mddev->flags)) { + ret = -EBUSY; + mutex_unlock(&mddev->reconfig_mutex); + } + + return ret; }
There are also other helpers like mddev_lock_nointr, mddev_suspend_and_lock_nointr, mddev_trylock. Thanks, Kuai
/* Sometimes we need to take the lock in a situation where
* failure due to interrupts is not acceptable.
+ * It doesn't need to check MD_DELETED here, the owner which
+ * holds the lock here can't be stopped.
*/
static inline void mddev_lock_nointr(struct mddev *mddev)
{
