From 88b199c24e789f3680193d2f41101f75efd8803f Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Tue, 27 Sep 2022 19:45:51 -0400 Subject: [PATCH] Cleanup spa_export_common() Coverity complains about a possible NULL pointer dereference. This is impossible, but it suspects it because we do a NULL check against `spa->spa_root_vdev`. This NULL check was never necessary and makes the code harder to understand, so we drop it. In particular, we dereference `spa->spa_root_vdev` when `new_state != POOL_STATE_UNINITIALIZED && !hardforce`. The first is only true when spa_reset is called, which only occurs under fault injection. The second is true unless `zpool export -F $POOLNAME` is used. Therefore, we effectively *always* dereference the pointer. In the cases where we do not, there is no reason to think it is unsafe. Therefore this change is safe to make. Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #13905 --- module/zfs/spa.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/module/zfs/spa.c b/module/zfs/spa.c index 10efb36f00..b8e054a5f2 100644 --- a/module/zfs/spa.c +++ b/module/zfs/spa.c @@ -6427,6 +6427,7 @@ spa_export_common(const char *pool, int new_state, nvlist_t **oldconfig, } if (spa->spa_sync_on) { + vdev_t *rvd = spa->spa_root_vdev; /* * A pool cannot be exported if it has an active shared spare. * This is to prevent other pools stealing the active spare @@ -6446,13 +6447,10 @@ spa_export_common(const char *pool, int new_state, nvlist_t **oldconfig, * dirty data resulting from the initialization is * committed to disk before we unload the pool. */ - if (spa->spa_root_vdev != NULL) { - vdev_t *rvd = spa->spa_root_vdev; - vdev_initialize_stop_all(rvd, VDEV_INITIALIZE_ACTIVE); - vdev_trim_stop_all(rvd, VDEV_TRIM_ACTIVE); - vdev_autotrim_stop_all(spa); - vdev_rebuild_stop_all(spa); - } + vdev_initialize_stop_all(rvd, VDEV_INITIALIZE_ACTIVE); + vdev_trim_stop_all(rvd, VDEV_TRIM_ACTIVE); + vdev_autotrim_stop_all(spa); + vdev_rebuild_stop_all(spa); /* * We want this to be reflected on every label, @@ -6462,7 +6460,7 @@ spa_export_common(const char *pool, int new_state, nvlist_t **oldconfig, if (new_state != POOL_STATE_UNINITIALIZED && !hardforce) { spa_config_enter(spa, SCL_ALL, FTAG, RW_WRITER); spa->spa_state = new_state; - vdev_config_dirty(spa->spa_root_vdev); + vdev_config_dirty(rvd); spa_config_exit(spa, SCL_ALL, FTAG); }