Skip to content

zcp: get_prop: fix encryptionroot and encryption #17280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

zcp: get_prop: fix encryptionroot and encryption #17280

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion module/zfs/zcp_get.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,14 +378,17 @@ get_special_prop(lua_State *state, dsl_dataset_t *ds, const char *dsname,
break;
}

case ZFS_PROP_ENCRYPTION:
case ZFS_PROP_KEYSTATUS:
case ZFS_PROP_KEYFORMAT: {
/* provide defaults in case no crypto obj exists */
setpoint[0] = '\0';
if (zfs_prop == ZFS_PROP_KEYSTATUS)
numval = ZFS_KEYSTATUS_NONE;
else
else if (zfs_prop == ZFS_PROP_KEYFORMAT)
numval = ZFS_KEYFORMAT_NONE;
else if (zfs_prop == ZFS_PROP_ENCRYPTION)
numval = ZIO_CRYPT_OFF;

nvlist_t *nvl, *propval;
nvl = fnvlist_alloc();
Expand All @@ -404,6 +407,25 @@ get_special_prop(lua_State *state, dsl_dataset_t *ds, const char *dsname,
break;
}

case ZFS_PROP_ENCRYPTION_ROOT: {
setpoint[0] = '\0';
numval = 0;
Copy link
Member

@amotin amotin Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why do you initialize numval here, not strval?

And also wonder why you are not setting setpoint below, similar to above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numval is a leftover, can delete that;

filling setpoint with the path didn't work (the result I was getting was empty AFAIK) - I wasn't clear on what setpoint is for, intuitively I would say that it's relevant when the value is numerical or if there's the need to pass another string along; looking at dsl_get_mountpoint, its impl refers to the third parameter as source

originally I tried to lump ZFS_PROP_ENCRYPTION_ROOT with the rest of the pack above but those are all numeric values

if you check the other properties in this function above, some set only strval and only a few set both setpoint and strval

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amotin while we're at this, what do you think of this:

  • if you do zfs get all, then encryptionroot isn't listed there
  • but zfs get encryptionroot works
  • I know some properties might be intentionally hidden, those are AFAIK registered with the zprop_register_hidden variant - but ZFS_PROP_ENCRYPTION_ROOT is registered with zprop_register_string, so that should be visible in get all, shouldn't it?

should I file an issue for that or am I missing something and it should in fact be hidden?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it is named so, but in the code above setpoint seems to be set to the property source, which is "default", "local", "received", etc. And unless it somehow makes no sense for ZFS_PROP_ENCRYPTION_ROOT, it would be nice to report it too.

I am not aware why ZFS_PROP_ENCRYPTION_ROOT might be hidden. I can think of two reasons to hide properties: either they are too technical to expose to user, but still somehow useful for some tools, or it is too expensive to fetch. I don't think either applies to ZFS_PROP_ENCRYPTION_ROOT, but I might be wrong on the first side, since I am not too deep in encryption administration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will take another look at setpoint;

btw @ zfs get all, apparently a property that doesn't have a value at all isn't listed in get all, although it is accessible via direct get; if I do get all with a dataset that does have encryptionroot, it works


nvlist_t *nvl, *propval;
nvl = fnvlist_alloc();
dsl_dataset_crypt_stats(ds, nvl);
if (nvlist_lookup_nvlist(nvl, zfs_prop_to_name(zfs_prop),
&propval) == 0) {
const char *dsname;

if (nvlist_lookup_string(propval, ZPROP_VALUE,
&dsname) == 0)
strlcpy(strval, dsname, ZAP_MAXVALUELEN);
}
nvlist_free(nvl);
break;
}

case ZFS_PROP_SNAPSHOTS_CHANGED:
numval = dsl_dir_snap_cmtime(ds->ds_dir).tv_sec;
break;
Expand Down