Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 21 Sep 2023 22:21:40 GMT
From:      Alan Somers <asomers@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 3676929d3cae - stable/13 - Fix zfsd with the device_removal pool feature.
Message-ID:  <202309212221.38LMLeOh088481@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by asomers:

URL: https://cgit.FreeBSD.org/src/commit/?id=3676929d3caea131170a48f8e054299d68c81ec5

commit 3676929d3caea131170a48f8e054299d68c81ec5
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2023-09-12 01:20:39 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2023-09-21 22:20:43 +0000

    Fix zfsd with the device_removal pool feature.
    
    Previously zfsd would crash in the presence of a pool with a
    top-level-vdev that had previously been removed.  The crash happened
    because the configuration nvlist of such a TLV contains an empty
    ZPOOL_CONFIG_CHILDREN array, which led to a pop_front from an empty
    list, which has undefined behavior.
    
    The crash only happened in stable/14 and later, probably do to
    differences in libcxx, but the change should be MFCed anyway.
    
    PR:             273663
    Reported by:    Marek Zarychta <zarychtam@plan-b.pwste.edu.pl>
    Sponsored by:   Axcient
    Reviewed by:    mav
    Differential Revision: https://reviews.freebsd.org/D41818
    
    (cherry picked from commit 0b294a386d34f6584848ed52407687df7ae59861)
---
 cddl/usr.sbin/zfsd/tests/zfsd_unittest.cc | 37 +++++++++++++++++++++++++++++++
 cddl/usr.sbin/zfsd/vdev_iterator.cc       |  5 +----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/cddl/usr.sbin/zfsd/tests/zfsd_unittest.cc b/cddl/usr.sbin/zfsd/tests/zfsd_unittest.cc
index f3fea2ca83f4..caeb077a3de8 100644
--- a/cddl/usr.sbin/zfsd/tests/zfsd_unittest.cc
+++ b/cddl/usr.sbin/zfsd/tests/zfsd_unittest.cc
@@ -766,3 +766,40 @@ TEST_F(ReEvaluateByGuidTest, ReEvaluateByGuid_five)
 	delete CaseFile4;
 	delete CaseFile5;
 }
+
+/*
+ * Test VdevIterator
+ */
+class VdevIteratorTest : public ::testing::Test
+{
+};
+
+bool VdevIteratorTestCB(Vdev &vdev, void *cbArg) {
+	return (false);
+}
+
+/*
+ * VdevIterator::Next should not crash when run on a pool that has a previously
+ * removed vdev.  Regression for
+ * https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=273663
+ */
+TEST_F(VdevIteratorTest, VdevRemoval)
+{
+	nvlist_t* poolConfig, *rootVdev;
+
+	ASSERT_EQ(0, nvlist_alloc(&rootVdev, NV_UNIQUE_NAME, 0));
+	ASSERT_EQ(0, nvlist_add_uint64(rootVdev, ZPOOL_CONFIG_GUID, 0x5678));
+	/*
+	 * Note: pools with previously-removed top-level VDEVs will contain a
+	 * TLV in their labels that has 0 children.
+	 */
+	ASSERT_EQ(0, nvlist_add_nvlist_array(rootVdev, ZPOOL_CONFIG_CHILDREN,
+				NULL, 0));
+	ASSERT_EQ(0, nvlist_alloc(&poolConfig, NV_UNIQUE_NAME, 0));
+	ASSERT_EQ(0, nvlist_add_uint64(poolConfig,
+			ZPOOL_CONFIG_POOL_GUID, 0x1234));
+	ASSERT_EQ(0, nvlist_add_nvlist(poolConfig, ZPOOL_CONFIG_VDEV_TREE,
+				rootVdev));
+
+	VdevIterator(poolConfig).Each(VdevIteratorTestCB, NULL);
+}
diff --git a/cddl/usr.sbin/zfsd/vdev_iterator.cc b/cddl/usr.sbin/zfsd/vdev_iterator.cc
index 5a515f91e2ec..e2294856b87d 100644
--- a/cddl/usr.sbin/zfsd/vdev_iterator.cc
+++ b/cddl/usr.sbin/zfsd/vdev_iterator.cc
@@ -100,10 +100,7 @@ VdevIterator::Next()
 {
 	nvlist_t *vdevConfig;
 
-	if (m_vdevQueue.empty())
-		return (NULL);
-
-	for (;;) {
+	for (vdevConfig = NULL; !m_vdevQueue.empty();) {
 		nvlist_t **vdevChildren;
 		int        result;
 		u_int      numChildren;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202309212221.38LMLeOh088481>