From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2464114A0B7 for ; Tue, 3 Jun 2025 05:20:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748928039; cv=none; b=Mt1Fy8LJMHsLRLc7a1RYr/T9zQscoEHl7QRkTvftMtGDG3pyv9XvvlDGjfKL2XJr9h2G9aDjRUH0q6dYk4iQHVtxw0zpP/3cjqwaxDvwaGcWfkdoUuBc0vCLFfKhvyXcTWjKCjpzGqVbzF3jSrVNS68h3pmUk2PVXVPQpDgEKiU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748928039; c=relaxed/simple; bh=pG6hWtbnKK9YiMqGrD2Y0XISrmtvrh7rN/va/lRcwvA=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=cBZ/mhjGlHsDuaDFwjKx12yVg9SZ6oIdVtZGwY0WJjPu7b3dLHqcW6IUCSRJAI6yP1/llENr2Ibj2HoXdugE/yF1w1tzHvWFbrAwe1EmWJ/rbgQKH09ZNcP5vG2pYoDlkzXfGCYp+voFLZcmWxk5XUADLqPeAe0EwxZxAIwMgEg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=P1sRWUJj; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="P1sRWUJj" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1748928037; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GO9Y6UguPxT4czIqS0eEzVtyPvXniZVZXzSVBIXX+6c=; b=P1sRWUJjLzX1dI1V21drZy3VwpF43Pn+5xfFvjbkRqOjPReTUqSyqO3eBFhW6lMwBDwQpr tlEpwH7+uono4XcpuuRVM02yfD3uVbQr9WwlTty1t1WxT752u86jRYEsrfV2U5OvfOXrG1 hUfUTp4KlM+ejUJ0Op/bO51ZXveiYWk= Received: from mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (ec2-35-165-154-97.us-west-2.compute.amazonaws.com [35.165.154.97]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-34-vojCxhyqP8qa_S4J-AfKaQ-1; Tue, 03 Jun 2025 01:20:33 -0400 X-MC-Unique: vojCxhyqP8qa_S4J-AfKaQ-1 X-Mimecast-MFC-AGG-ID: vojCxhyqP8qa_S4J-AfKaQ_1748928032 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id A8B1C1800446; Tue, 3 Jun 2025 05:20:32 +0000 (UTC) Received: from localhost.localdomain (unknown [10.72.120.30]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id D7EA830002C4; Tue, 3 Jun 2025 05:20:29 +0000 (UTC) From: Xiao Ni To: linux-raid@vger.kernel.org Cc: yukuai3@huawei.com, song@kernel.org, ncroxon@redhat.com Subject: [PATCH 1/3] md: call del_gendisk in control path Date: Tue, 3 Jun 2025 13:20:21 +0800 Message-Id: <20250603052023.7922-2-xni@redhat.com> In-Reply-To: <20250603052023.7922-1-xni@redhat.com> References: <20250603052023.7922-1-xni@redhat.com> Precedence: bulk X-Mailing-List: linux-raid@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 Now del_gendisk and put_disk are called asynchronously in workqueue work. The asynchronous way 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 of gendisk. Function del_gendisk can't be called with reconfig_mutex. If it's called with reconfig mutex, a deadlock can happen. del_gendisk waits all sysfs files access to finish and sysfs file access waits reconfig mutex. So put del_gendisk after releasing reconfig mutex. But there is still 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. MD_DELETED is used to resolve this problem. MD_DELETED is set before releasing reconfig mutex and it should be checked for these sysfs access which need reconfig mutex. For sysfs access which don't need reconfig mutex, del_gendisk will wait them to finish. But it doesn't need to do this in function mddev_lock_nointr. There are ten places that call it. * Five of them are in dm raid which we don't need to care. MD_DELETED is only used for md raid. * stop_sync_thread, md_do_sync and md_start_sync are related sync request, and it needs to wait sync thread to finish before stopping an array. * md_ioctl: md_open is called before md_ioctl, so ->openers is added. It will fail to stop the array. So it doesn't need to check MD_DELETED here * md_set_readonly: It needs to call mddev_set_closing_and_sync_blockdev when setting readonly or read_auto. So it will fail to stop the array too because MD_CLOSING is already set. Signed-off-by: Xiao Ni --- v3: call del_gendisk in mddev_unlock drivers/md/md.c | 28 +++++++++++++++++++++++----- drivers/md/md.h | 26 ++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 9daa78c5fe33..fd6c5ff0e3c1 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -613,9 +613,6 @@ static void __mddev_put(struct mddev *mddev) 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); - /* * Call queue_work inside the spinlock so that flush_workqueue() after * mddev_find will succeed in waiting for the work to be done. @@ -850,6 +847,16 @@ void mddev_unlock(struct mddev *mddev) kobject_del(&rdev->kobj); export_rdev(rdev, mddev); } + + /* Call del_gendisk after release reconfig_mutex to avoid + * deadlock (e.g. call del_gendisk under the lock and an + * access to sysfs files waits the lock) + * And MD_DELETED is only used for md raid which is set in + * do_md_stop. dm raid only uses md_stop to stop. So dm raid + * doesn't need to check MD_DELETED when getting reconfig lock + */ + if (test_bit(MD_DELETED, &mddev->flags)) + del_gendisk(mddev->gendisk); } EXPORT_SYMBOL_GPL(mddev_unlock); @@ -5721,19 +5728,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 +5764,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); } @@ -6593,8 +6610,9 @@ 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); + if (mddev->hold_active == UNTIL_STOP) mddev->hold_active = 0; } diff --git a/drivers/md/md.h b/drivers/md/md.h index 1cf00a04bcdd..ef5e4a8f8e71 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -697,11 +697,26 @@ 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. + */ + if (!ret && test_bit(MD_DELETED, &mddev->flags)) { + ret = -EBUSY; + mutex_unlock(&mddev->reconfig_mutex); + } + + return ret; } /* 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. And all paths can't + * call this function after do_md_stop. */ static inline void mddev_lock_nointr(struct mddev *mddev) { @@ -710,7 +725,14 @@ static inline void mddev_lock_nointr(struct mddev *mddev) static inline int mddev_trylock(struct mddev *mddev) { - return mutex_trylock(&mddev->reconfig_mutex); + int ret = 0; + + ret = mutex_trylock(&mddev->reconfig_mutex); + if (!ret && test_bit(MD_DELETED, &mddev->flags)) { + ret = -EBUSY; + mutex_unlock(&mddev->reconfig_mutex); + } + return ret; } extern void mddev_unlock(struct mddev *mddev); -- 2.32.0 (Apple Git-132)