Skip to content

Commit

Permalink
spa_prop_get: require caller to supply output nvlist
Browse files Browse the repository at this point in the history 
All callers to spa_prop_get() and spa_prop_get_nvlist() supplied their
own preallocated nvlist (except ztest), so we can remove the option to
have them allocate one if none is supplied.

This sidesteps a bug in spa_prop_get(), where the error var wasn't
initialised, which could lead to the provided nvlist being freed at the
end.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
  • Loading branch information
@robn
robn committed Sep 4, 2024
1 parent bf8c61f commit 366d6fe
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 66 deletions.
5 changes: 3 additions & 2 deletions cmd/ztest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6211,13 +6211,14 @@ void
ztest_spa_prop_get_set(ztest_ds_t *zd, uint64_t id)
{
(void) zd, (void) id;
nvlist_t *props = NULL;

(void) pthread_rwlock_rdlock(&ztest_name_lock);

(void) ztest_spa_prop_set_uint64(ZPOOL_PROP_AUTOTRIM, ztest_random(2));

VERIFY0(spa_prop_get(ztest_spa, &props));
nvlist_t *props = fnvlist_alloc();

VERIFY0(spa_prop_get(ztest_spa, props));

if (ztest_opts.zo_verbose >= 6)
dump_nvlist(props, 4);
Expand Down
4 changes: 2 additions & 2 deletions include/sys/spa.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1201,9 +1201,9 @@ extern void spa_boot_init(void);

/* properties */
extern int spa_prop_set(spa_t *spa, nvlist_t *nvp);
extern int spa_prop_get(spa_t *spa, nvlist_t **nvp);
extern int spa_prop_get(spa_t *spa, nvlist_t *nvp);
extern int spa_prop_get_nvlist(spa_t *spa, char **props,
unsigned int n_props, nvlist_t **outnvl);
unsigned int n_props, nvlist_t *outnvl);
extern void spa_prop_clear_bootfs(spa_t *spa, uint64_t obj, dmu_tx_t *tx);
extern void spa_configfile_set(spa_t *, nvlist_t *, boolean_t);

Expand Down
100 changes: 43 additions & 57 deletions module/zfs/spa.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,21 +366,15 @@ spa_prop_add(spa_t *spa, const char *propname, nvlist_t *outnvl)

int
spa_prop_get_nvlist(spa_t *spa, char **props, unsigned int n_props,
nvlist_t **outnvl)
nvlist_t *outnvl)
{
int err = 0;

if (props == NULL)
return (0);

if (*outnvl == NULL) {
err = nvlist_alloc(outnvl, NV_UNIQUE_NAME, KM_SLEEP);
if (err)
return (err);
}

for (unsigned int i = 0; i < n_props && err == 0; i++) {
err = spa_prop_add(spa, props[i], *outnvl);
err = spa_prop_add(spa, props[i], outnvl);
}

return (err);
Expand All @@ -406,7 +400,7 @@ spa_prop_add_user(nvlist_t *nvl, const char *propname, char *strval,
* Get property values from the spa configuration.
*/
static void
spa_prop_get_config(spa_t *spa, nvlist_t **nvp)
spa_prop_get_config(spa_t *spa, nvlist_t *nv)
{
vdev_t *rvd = spa->spa_root_vdev;
dsl_pool_t *pool = spa->spa_dsl_pool;
Expand All @@ -428,48 +422,48 @@ spa_prop_get_config(spa_t *spa, nvlist_t **nvp)
size += metaslab_class_get_space(spa_dedup_class(spa));
size += metaslab_class_get_space(spa_embedded_log_class(spa));

spa_prop_add_list(*nvp, ZPOOL_PROP_NAME, spa_name(spa), 0, src);
spa_prop_add_list(*nvp, ZPOOL_PROP_SIZE, NULL, size, src);
spa_prop_add_list(*nvp, ZPOOL_PROP_ALLOCATED, NULL, alloc, src);
spa_prop_add_list(*nvp, ZPOOL_PROP_FREE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_NAME, spa_name(spa), 0, src);
spa_prop_add_list(nv, ZPOOL_PROP_SIZE, NULL, size, src);
spa_prop_add_list(nv, ZPOOL_PROP_ALLOCATED, NULL, alloc, src);
spa_prop_add_list(nv, ZPOOL_PROP_FREE, NULL,
size - alloc, src);
spa_prop_add_list(*nvp, ZPOOL_PROP_CHECKPOINT, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_CHECKPOINT, NULL,
spa->spa_checkpoint_info.sci_dspace, src);

spa_prop_add_list(*nvp, ZPOOL_PROP_FRAGMENTATION, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_FRAGMENTATION, NULL,
metaslab_class_fragmentation(mc), src);
spa_prop_add_list(*nvp, ZPOOL_PROP_EXPANDSZ, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_EXPANDSZ, NULL,
metaslab_class_expandable_space(mc), src);
spa_prop_add_list(*nvp, ZPOOL_PROP_READONLY, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_READONLY, NULL,
(spa_mode(spa) == SPA_MODE_READ), src);

cap = (size == 0) ? 0 : (alloc * 100 / size);
spa_prop_add_list(*nvp, ZPOOL_PROP_CAPACITY, NULL, cap, src);
spa_prop_add_list(nv, ZPOOL_PROP_CAPACITY, NULL, cap, src);

spa_prop_add_list(*nvp, ZPOOL_PROP_DEDUPRATIO, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_DEDUPRATIO, NULL,
ddt_get_pool_dedup_ratio(spa), src);
spa_prop_add_list(*nvp, ZPOOL_PROP_BCLONEUSED, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_BCLONEUSED, NULL,
brt_get_used(spa), src);
spa_prop_add_list(*nvp, ZPOOL_PROP_BCLONESAVED, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_BCLONESAVED, NULL,
brt_get_saved(spa), src);
spa_prop_add_list(*nvp, ZPOOL_PROP_BCLONERATIO, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_BCLONERATIO, NULL,
brt_get_ratio(spa), src);

spa_prop_add_list(*nvp, ZPOOL_PROP_DEDUP_TABLE_SIZE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_DEDUP_TABLE_SIZE, NULL,
ddt_get_ddt_dsize(spa), src);

spa_prop_add_list(*nvp, ZPOOL_PROP_HEALTH, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_HEALTH, NULL,
rvd->vdev_state, src);

version = spa_version(spa);
if (version == zpool_prop_default_numeric(ZPOOL_PROP_VERSION)) {
spa_prop_add_list(*nvp, ZPOOL_PROP_VERSION, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_VERSION, NULL,
version, ZPROP_SRC_DEFAULT);
} else {
spa_prop_add_list(*nvp, ZPOOL_PROP_VERSION, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_VERSION, NULL,
version, ZPROP_SRC_LOCAL);
}
spa_prop_add_list(*nvp, ZPOOL_PROP_LOAD_GUID,
spa_prop_add_list(nv, ZPOOL_PROP_LOAD_GUID,
NULL, spa_load_guid(spa), src);
}

Expand All @@ -479,62 +473,62 @@ spa_prop_get_config(spa_t *spa, nvlist_t **nvp)
* when opening pools before this version freedir will be NULL.
*/
if (pool->dp_free_dir != NULL) {
spa_prop_add_list(*nvp, ZPOOL_PROP_FREEING, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_FREEING, NULL,
dsl_dir_phys(pool->dp_free_dir)->dd_used_bytes,
src);
} else {
spa_prop_add_list(*nvp, ZPOOL_PROP_FREEING,
spa_prop_add_list(nv, ZPOOL_PROP_FREEING,
NULL, 0, src);
}

if (pool->dp_leak_dir != NULL) {
spa_prop_add_list(*nvp, ZPOOL_PROP_LEAKED, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_LEAKED, NULL,
dsl_dir_phys(pool->dp_leak_dir)->dd_used_bytes,
src);
} else {
spa_prop_add_list(*nvp, ZPOOL_PROP_LEAKED,
spa_prop_add_list(nv, ZPOOL_PROP_LEAKED,
NULL, 0, src);
}
}

spa_prop_add_list(*nvp, ZPOOL_PROP_GUID, NULL, spa_guid(spa), src);
spa_prop_add_list(nv, ZPOOL_PROP_GUID, NULL, spa_guid(spa), src);

if (spa->spa_comment != NULL) {
spa_prop_add_list(*nvp, ZPOOL_PROP_COMMENT, spa->spa_comment,
spa_prop_add_list(nv, ZPOOL_PROP_COMMENT, spa->spa_comment,
0, ZPROP_SRC_LOCAL);
}

if (spa->spa_compatibility != NULL) {
spa_prop_add_list(*nvp, ZPOOL_PROP_COMPATIBILITY,
spa_prop_add_list(nv, ZPOOL_PROP_COMPATIBILITY,
spa->spa_compatibility, 0, ZPROP_SRC_LOCAL);
}

if (spa->spa_root != NULL)
spa_prop_add_list(*nvp, ZPOOL_PROP_ALTROOT, spa->spa_root,
spa_prop_add_list(nv, ZPOOL_PROP_ALTROOT, spa->spa_root,
0, ZPROP_SRC_LOCAL);

if (spa_feature_is_enabled(spa, SPA_FEATURE_LARGE_BLOCKS)) {
spa_prop_add_list(*nvp, ZPOOL_PROP_MAXBLOCKSIZE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_MAXBLOCKSIZE, NULL,
MIN(zfs_max_recordsize, SPA_MAXBLOCKSIZE), ZPROP_SRC_NONE);
} else {
spa_prop_add_list(*nvp, ZPOOL_PROP_MAXBLOCKSIZE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_MAXBLOCKSIZE, NULL,
SPA_OLD_MAXBLOCKSIZE, ZPROP_SRC_NONE);
}

if (spa_feature_is_enabled(spa, SPA_FEATURE_LARGE_DNODE)) {
spa_prop_add_list(*nvp, ZPOOL_PROP_MAXDNODESIZE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_MAXDNODESIZE, NULL,
DNODE_MAX_SIZE, ZPROP_SRC_NONE);
} else {
spa_prop_add_list(*nvp, ZPOOL_PROP_MAXDNODESIZE, NULL,
spa_prop_add_list(nv, ZPOOL_PROP_MAXDNODESIZE, NULL,
DNODE_MIN_SIZE, ZPROP_SRC_NONE);
}

if ((dp = list_head(&spa->spa_config_list)) != NULL) {
if (dp->scd_path == NULL) {
spa_prop_add_list(*nvp, ZPOOL_PROP_CACHEFILE,
spa_prop_add_list(nv, ZPOOL_PROP_CACHEFILE,
"none", 0, ZPROP_SRC_LOCAL);
} else if (strcmp(dp->scd_path, spa_config_path) != 0) {
spa_prop_add_list(*nvp, ZPOOL_PROP_CACHEFILE,
spa_prop_add_list(nv, ZPOOL_PROP_CACHEFILE,
dp->scd_path, 0, ZPROP_SRC_LOCAL);
}
}
Expand All @@ -544,19 +538,13 @@ spa_prop_get_config(spa_t *spa, nvlist_t **nvp)
* Get zpool property values.
*/
int
spa_prop_get(spa_t *spa, nvlist_t **nvp)
spa_prop_get(spa_t *spa, nvlist_t *nv)
{
objset_t *mos = spa->spa_meta_objset;
zap_cursor_t zc;
zap_attribute_t za;
dsl_pool_t *dp;
int err;

if (*nvp == NULL) {
err = nvlist_alloc(nvp, NV_UNIQUE_NAME, KM_SLEEP);
if (err)
return (err);
}
int err = 0;

dp = spa_get_dsl(spa);
dsl_pool_config_enter(dp, FTAG);
Expand All @@ -565,7 +553,7 @@ spa_prop_get(spa_t *spa, nvlist_t **nvp)
/*
* Get properties from the spa config.
*/
spa_prop_get_config(spa, nvp);
spa_prop_get_config(spa, nv);

/* If no pool property object, no more prop to get. */
if (mos == NULL || spa->spa_pool_props_object == 0)
Expand Down Expand Up @@ -610,7 +598,7 @@ spa_prop_get(spa_t *spa, nvlist_t **nvp)
intval = za.za_first_integer;
}

spa_prop_add_list(*nvp, prop, strval, intval, src);
spa_prop_add_list(nv, prop, strval, intval, src);

if (strval != NULL)
kmem_free(strval, ZFS_MAX_DATASET_NAME_LEN);
Expand All @@ -627,10 +615,10 @@ spa_prop_get(spa_t *spa, nvlist_t **nvp)
break;
}
if (prop != ZPOOL_PROP_INVAL) {
spa_prop_add_list(*nvp, prop, strval, 0, src);
spa_prop_add_list(nv, prop, strval, 0, src);
} else {
src = ZPROP_SRC_LOCAL;
spa_prop_add_user(*nvp, za.za_name, strval,
spa_prop_add_user(nv, za.za_name, strval,
src);
}
kmem_free(strval, za.za_num_integers);
Expand All @@ -644,11 +632,9 @@ spa_prop_get(spa_t *spa, nvlist_t **nvp)
out:
mutex_exit(&spa->spa_props_lock);
dsl_pool_config_exit(dp, FTAG);
if (err && err != ENOENT) {
nvlist_free(*nvp);
*nvp = NULL;

if (err && err != ENOENT)
return (err);
}

return (0);
}
Expand Down
10 changes: 5 additions & 5 deletions module/zfs/zfs_ioctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3050,7 +3050,6 @@ static const zfs_ioc_key_t zfs_keys_get_props[] = {
static int
zfs_ioc_pool_get_props(const char *pool, nvlist_t *innvl, nvlist_t *outnvl)
{
nvlist_t *nvp = outnvl;
spa_t *spa;
char **props = NULL;
unsigned int n_props = 0;
Expand All @@ -3069,16 +3068,17 @@ zfs_ioc_pool_get_props(const char *pool, nvlist_t *innvl, nvlist_t *outnvl)
*/
mutex_enter(&spa_namespace_lock);
if ((spa = spa_lookup(pool)) != NULL) {
error = spa_prop_get(spa, &nvp);
error = spa_prop_get(spa, outnvl);
if (error == 0 && props != NULL)
error = spa_prop_get_nvlist(spa, props, n_props,
&nvp);
outnvl);
}
mutex_exit(&spa_namespace_lock);
} else {
error = spa_prop_get(spa, &nvp);
error = spa_prop_get(spa, outnvl);
if (error == 0 && props != NULL)
error = spa_prop_get_nvlist(spa, props, n_props, &nvp);
error = spa_prop_get_nvlist(spa, props, n_props,
outnvl);
spa_close(spa, FTAG);
}

Expand Down

0 comments on commit 366d6fe

Please sign in to comment.